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

New single drag and drop single FileUploader #14660

Merged
merged 18 commits into from
Oct 3, 2023

Conversation

guidari
Copy link
Contributor

@guidari guidari commented Sep 15, 2023

Closes #14502

The new story should upload only one file at a time and the component flow should follow these steps on the linked image in the issue. A new addition is the inclusion of a "Replace" button, which serves as a clear indicator to the user that only a single image is accepted.

Changelog

New

  • New story for Drag and Drop Single File Uploader

Testing / Reviewing

  • Go to the FileUploader in the Drag And Drop Upload Container Example Application variant and test if the functionality is working as expected.

@netlify
Copy link

netlify bot commented Sep 15, 2023

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 1bd87ea
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/651b26c6348c560008a4c94e
😎 Deploy Preview https://deploy-preview-14660--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 15, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 1bd87ea
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/651b26c6da77a90008ff3b69
😎 Deploy Preview https://deploy-preview-14660--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@guidari guidari requested review from a team and laurenmrice and removed request for a team September 18, 2023 19:20
@guidari guidari marked this pull request as ready for review September 18, 2023 19:20
@guidari guidari requested review from a team as code owners September 18, 2023 19:20
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

I don't know how much we need to account for this - but maybe @laurenmrice has design thoughts on how we can make this mobile friendly. It currently breaks at 350px

Screenshot 2023-09-18 at 23 19 10

@guidari
Copy link
Contributor Author

guidari commented Sep 19, 2023

I don't know how much we need to account for this - but maybe @laurenmrice has design thoughts on how we can make this mobile friendly. It currently breaks at 350px

Ohh good catch!! I totally forgot to test on mobile screens!
We can shorter the file name so the close button will fit or get rid of the "Replace" option.

Screenshot 2023-09-19 at 09 21 47

@guidari
Copy link
Contributor Author

guidari commented Sep 19, 2023

@laurenmrice There are a few pixels within the image in the issue that are sized at 24px instead of the required 16px. As the FileUploader component already exists, I didn't change those specific pixel dimensions.

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

this is what I'm seeing in firefox, I think it has to do with it needing a width or max-width for text-overflow: ellipsis; to work correctly in that browser.

Screenshot 2023-09-19 at 12 38 44 PM

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

@guidari Looking good Gui! ⭐️ Just noticed a couple of things:


“Replace” link:

  • This should be using the link component, not a ghost button.

Spacing:

  • There should be 16px padding to the right of the close icon. I think this is what you were referring to that it is currently 24px. It really should be 16px for both variants of drag/drop file uploader (the multiple file uploader one and the single file uploader one). This will also affect the positioning of the loading and success states, which should have 16px padding to the right.
Frame 4

Overflow text in file name:

  • When a file uploads with a long string a text as its File name, it currently looks how Alison showed in her image example. There should be 16px padding to the left of the Replacelink, and then we can ellipsis the file name. Here is an example:
Frame 3

File height size:

  • The sm prop currently does not change the height size of the file uploaded to 32px.

Mobile:

  • I like your idea for Mobile sizing, maybe it will work with following the same rules at the Overflow text I mentioned above^.

@guidari
Copy link
Contributor Author

guidari commented Sep 21, 2023

Thanks for the feedback guys! 🚀

“Replace” link:

  • This should be using the link component, not a ghost button.

@laurenmrice in this case we can't use the Link component because we are using the FileUploaderButton component to take advantage of all the logic behind uploading a file.

But we can replicate the style of a Link component in the button.

I see two ways of doing that:

1 - We can create a buttonKind called "link" (or something else)
2 - I can override the css without creating a new buttonKind just for the FileUploaderItem

Both ways we have a button that looks like a Link component.

I'm just worried about the number 1 because I'm not sure if a buttonKind called "link" would make sense. The prop size wouldn't affect this type of button.

Of course, we could also work on top of the Ghost kind as a third option.

Wondering what you guys think @laurenmrice @andreancardona @alisonjoseph

For now I feel like option 2 sounds good.

@alisonjoseph
Copy link
Member

@laurenmrice are we sure we want to use a Link component here?
https://carbondesignsystem.com/components/link/usage/#when-not-to-use

Use a button instead of a link for actions that will change data or manipulate how it is displayed, change a state, or trigger an action. Buttons should never be used for navigational actions.

Can we make ghost button work? Or do we need a new Button variant? Or should this just be a one-off set of styles for a button like @guidari mentioned?

@laurenmrice
Copy link
Member

@alisonjoseph @guidari I am getting in touch with Michael G about using a ghost button instead just to get his a11y opinion. I will let you know the outcome and provide a new spec here for it to work off of if we need one. 👍

@laurenmrice
Copy link
Member

Following up on the convo above, after speaking with Michael and showing this in the design crit, we are making the design a little simpler without a replace button. I provided the new specs in the original issue and went over them with Gui.

@laurenmrice laurenmrice self-requested a review September 22, 2023 18:18
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

The functionality of this looks great! There are just a couple small visual things.


  • I believe this is a Firefox bug: the text in the uploaded file is a little too high in the container. The text should be center within the container for all sizes.
 Example spec below:
Frame 1
  • 
The close icon looks like it’s a couple pixels too far to the right. From the bounding box of the icon (which is 16px x 16px) there then should be 16px padding on the right. However, when the icon is focused, the focus border should still stay at 24px x 24px (this is currently correct in the preview). Example spec below:
Frame 2

@guidari
Copy link
Contributor Author

guidari commented Sep 27, 2023

@laurenmrice

  • I believe this is a Firefox bug: the text in the uploaded file is a little too high in the container. The text should be center within the container for all sizes.
 Example spec below:

For some reason on FF was 21.5px the height for the filename. I just fixed and now it is the same as chorme (18px)

  • The close icon looks like it’s a couple pixels too far to the right. From the bounding box of the icon (which is 16px x 16px) there then should be 16px padding on the right. However, when the icon is focused, the focus border should still stay at 24px x 24px (this is currently correct in the preview). Example spec below:

Fixed! There was a gap that was causing that!

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Hey @guidari ! I just reviewed your PR again and noticed the close icon is not 16px away from the right edge of the uploaded file. I was just wondering if your PR preview updated or not?


  • When I upload a file with a long overflowing text name, in all sizes (lg, md, sm) the file name is still a little high, maybe by 1px. This does not happen with a short/normal file name , that one looks good! Also related to this, when I switch the size to medium or small, the close icon comes outside of the file to the right. This is happening in Firefox/Chrome/Safari.
Screenshot 2023-09-28 at 11 30 36 AM
  • There is one issue with the error state in the large size where the file name and icons sit too low in the top field area. All the other sizes look good. This is happening in Firefox/Chrome/Safari.
Screenshot 2023-09-28 at 11 33 36 AM

@guidari
Copy link
Contributor Author

guidari commented Sep 28, 2023

@laurenmrice Fixed!
It's just weird the file name being 1px high 🤔 I added a margin-top to fix that.

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks great, Gui! ⭐️

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

@guidari guidari added this pull request to the merge queue Oct 3, 2023
Merged via the queue into carbon-design-system:main with commit 5ca370b Oct 3, 2023
17 checks passed
@guidari guidari deleted the 14502-file-uploader branch October 3, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single File Uploader: Implementation
4 participants