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 upload component #316

Merged
merged 16 commits into from
Oct 9, 2024
Merged

Add upload component #316

merged 16 commits into from
Oct 9, 2024

Conversation

mollpo
Copy link
Contributor

@mollpo mollpo commented Sep 2, 2024

No description provided.

Copy link

github-actions bot commented Sep 2, 2024

Preview link: https://316.react-ui.aboutbits.dev

@mollpo mollpo changed the base branch from main to zod-validation September 2, 2024 06:53
@mollpo mollpo changed the base branch from zod-validation to main September 2, 2024 06:53
@mollpo mollpo requested review from alexlanz and lukasvice September 2, 2024 06:59
Copy link
Contributor

@lukasvice lukasvice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work! I have added some suggestions and questions for discussion.

src/components/files/DeleteFileAction.tsx Outdated Show resolved Hide resolved
src/components/button/ResponsiveButtonIcon.tsx Outdated Show resolved Hide resolved
src/components/button/ResponsiveButtonIcon.tsx Outdated Show resolved Hide resolved
src/components/button/theme.ts Outdated Show resolved Hide resolved
src/components/files/DownloadFileAction.tsx Outdated Show resolved Hide resolved
fileUploadObject: FileUploadObject<CustomRemoteFile>,
) => {
setIsDeleting(true)
await axiosInstance.post('/delete')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a try/catch here to have a good example code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be in the catch in that case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We could just ignore it in this example and add a comment Handle error accordingly or something. Wdyt?

src/components/files/FileUpload.stories.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to export the actions

src/components/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don't have any icons in this lib. I see the following possibilities for this:

  1. Keep it for internal use only (no export)
  2. Export it as component for projects
  3. Remove it and require the user to set a custom spinnerIcon prop where needed.
  4. Do not use a custom animated icon, but use the Progress Activity icon from our icon lib and rotate it.

Maybe option 4 is the most consistend, otherwise I would go with option 2.

Wdyt @alexlanz?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that we do not have any custom icons in ReactUI, but we are using our Material Icons package right?

Allowing to add a custom spinner icon is a bit of an overkill or not? I would try to go with option 4, because then we stick to the same icons that we already use and the animation could be done with only CSS.

What do you both think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, option 4 sounds also good

@mollpo mollpo requested a review from lukasvice September 3, 2024 14:21
@mollpo mollpo self-assigned this Sep 17, 2024
Copy link
Contributor

@lukasvice lukasvice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one too many tsx's in that filename ;)

@mollpo mollpo merged commit b6242da into main Oct 9, 2024
4 checks passed
@mollpo mollpo deleted the add-upload-component branch October 9, 2024 11:11
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.

3 participants