-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
New single drag and drop single FileUploader
#14660
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this 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
Ohh good catch!! I totally forgot to test on mobile screens! |
@laurenmrice There are a few pixels within the image in the issue that are sized at 24px instead of the required 16px. As the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
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
Replace
link, and then we can ellipsis the file name. Here is an example:
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^.
Thanks for the feedback guys! 🚀
@laurenmrice in this case we can't use the But we can replicate the style of a I see two ways of doing that: 1 - We can create a Both ways we have a button that looks like a I'm just worried about the number 1 because I'm not sure if a 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. |
@laurenmrice are we sure we want to use a Link component here?
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? |
@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. 👍 |
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. |
There was a problem hiding this 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:
- 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:
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)
Fixed! There was a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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.
- 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.
@laurenmrice Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, Gui! ⭐️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🎉
5ca370b
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
Testing / Reviewing
FileUploader
in theDrag And Drop Upload Container Example Application
variant and test if the functionality is working as expected.