Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PackageUpload component #8

Merged
merged 40 commits into from
Apr 2, 2021
Merged

Add PackageUpload component #8

merged 40 commits into from
Apr 2, 2021

Conversation

nihaals
Copy link
Member

@nihaals nihaals commented Feb 21, 2021

Closes #39 and #46

@nihaals nihaals force-pushed the feature/package-upload branch from 2c19338 to 010700b Compare February 21, 2021 15:02
Also add Markdown, FileUpload and StickyFooter
@nihaals nihaals force-pushed the feature/package-upload branch from 010700b to 8ef74ed Compare February 27, 2021 14:44
@nihaals
Copy link
Member Author

nihaals commented Mar 2, 2021

A better explanation of the to-do list (related: #22 (comment)):

Add SelectSearch component

This is currently Select and uses an abstraction that should make changing the actual select component easier. Some points:

  • It needs to work with react hook form (relevant docs for controlled inputs)
  • It needs to support multiselect
    • I'm unsure if splitting multiselect into a separate component or if keeping it as a toggle with a prop is better

This is a component that has given me a lot of trouble. I really like the selectors in the existing upload form but finding one that "works with Chakra" has been difficult. The lack of components in Chakra like a date picker and multi select has made building these components really important but I haven't managed to find a good solution. koolamusic/chakra-ui-autocomplete seems to be the only existing select component that has everything that's needed but after a quick mess around I found it quite complex to work with (although this shouldn't be a big deal due to the abstraction of our own Select component). I personally really struggle with CSS and haven't got any experience with building "lower level"/highly custom components like a select and multiselect component so this has probably been the biggest roadblock. I also looked at react-select-search which has an example I really like but it requires styling or using a custom renderer which after a small test seemed still complex despite now being able to use Chakra components. If we do style it ourselves, it would be nice to have it use the Chakra theme's colours.

Move apiToken to localStorage

This should be fairly simple, it is currently part of a context but it should be stored in localStorage. This can be bundled with actually dynamically getting the token but this is blocked by the backend PR which will add an endpoint that can generate a token from a session ID cookie (only for the migration phase when these components will be used in Django templates).

Add data fetching

I'm currently looking at react-query and SWR. This is also blocked by the backend PR. This will replace the current hard coded example options, e.g.:

const options: SelectOption[] = [
{ name: "Category name 1", value: "category-slug-1" },
{ name: "Category name 2", value: "category-slug-2" },
{ name: "Category name 3", value: "category-slug-3" },
];

Remove top margin from first item in README

This is probably really simple but I haven't thought of a solution (probably my lack of CSS knowledge). Currently, if you upload a mod zip where the first line of the README is a header (which is common), the top margin of the header causes a weird gap between the header and the outline of the markdown preview box. Example:

image

Standardise background colours

Right now useColorModeValue is used in 2 places, the background of the footer with the upload button and the background of the file upload component.

image

image

Both of these currently use "gray", "gray" but there's probably a much better colour based on the theme that can be used to have a less drastic colour change from the background colour. I haven't looked into reading values from the Chakra theme or played around with different colours (also trying Chakra's colour system) to find what would work here but it doesn't really fit in well the way it is at the moment. I think the accent colour we use for these 2 places could be exported or stored in the theme so that it can easily be referenced, as already it has 2 references and this is just implementing one page. I'm not sure where this would be put but looking at how Chakra's theme works will probably help.

Change decompressing message to loading icon

Right now there are 2 slightly weird loading texts, "Loading..." and "Decompressing...". It would be nice to instead show a loading spinner here. It might also make sense to have some animations as the state changes but I'm not sure on the best way to do that.

Use Chakra form components

For example, FormControl, FormLabel and FormErrorMessage. When I experimented with FormControl it broke some of the styling but this should be a fairly simple change.

Fix typing for ref

At the moment, the ref used from register from react-hook-form is set to any instead of the correct type. This was more complex than I expected so I left it for later.

@nihaals
Copy link
Member Author

nihaals commented Mar 14, 2021

I'm unsure where to put API token fetching (currently /api/experimental/auth/token/) for Django, it might work best in the Django template where it's able to hook into logging in. It can then store the token in local storage before mounting the component.

@nihaals
Copy link
Member Author

nihaals commented Apr 2, 2021

I'm merging this since it's currently blocking a lot as it has much more than just the new components. Pending to-dos will be in a separate PR.

@nihaals nihaals marked this pull request as ready for review April 2, 2021 17:01
@nihaals nihaals merged commit 742759c into master Apr 2, 2021
@nihaals nihaals deleted the feature/package-upload branch April 2, 2021 17:03
@nihaals nihaals linked an issue Jul 2, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Select component Add package upload component
2 participants