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

fix(file-uploader): adjusts default button and removes clear file button #5166

Merged
merged 25 commits into from
Jan 31, 2020
Merged

fix(file-uploader): adjusts default button and removes clear file button #5166

merged 25 commits into from
Jan 31, 2020

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Jan 23, 2020

It doesn't close #3587 but checks off some of its points.

According to #3587, the "Upload Files" button should be our primary button not the small one so I removed ``${prefix}--btn--sm` from the list of class names. This also corrects the selectors for the file container so the margin is there. The issue states that the file container should be 40px instead of 32px so I changed the spacing token to $carbon--spacing-08. Also I removed the "Clear Files" button since the issue and the design spec show that it shouldn't be there.

Changelog

Changed

  • selector for file container
  • height of file container

Removed

  • "Clear files" button

Testing / Reviewing

Check to make sure the FileUploader looks how it should!

@abbeyhrt abbeyhrt requested a review from a team as a code owner January 23, 2020 23:32
@ghost ghost requested review from dakahn and joshblack January 23, 2020 23:32
@abbeyhrt abbeyhrt requested review from a team, jeanservaas and laurenmrice and removed request for a team and jeanservaas January 23, 2020 23:32
@netlify
Copy link

netlify bot commented Jan 23, 2020

Deploy preview for the-carbon-components ready!

Built with commit 49bdc4a

https://deploy-preview-5166--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 23, 2020

Deploy preview for carbon-elements ready!

Built with commit 49bdc4a

https://deploy-preview-5166--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 23, 2020

Deploy preview for carbon-components-react failed.

Built with commit 49bdc4a

https://app.netlify.com/sites/carbon-components-react/deploys/5e34b93114dbe800088d5823

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

just a couple things but mostly for design to confirm

@@ -112,7 +112,7 @@
grid-template-columns: 1fr auto;
grid-auto-rows: auto;
align-items: center;
min-height: $carbon--spacing-07;
min-height: $carbon--spacing-08;
Copy link
Member

Choose a reason for hiding this comment

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

want to double check if this change is across the board or for non drag-and-drop uploader only based on #2288 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurenmrice any thoughts? This is the part that changes the uploaded file from 32px -> 40px, so we just want to confirm if only the non-drag and drop variants should have the 40px uploaded file size!

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 48px looks good and clear button is gone👍🏻.
  • Since we now have the 48px input fields can we use that size for the uploaded files instead of 40px. When that issue was made we didn't have the 48px yet.
  • The close icon should be 16px away from the container and it should be calling the icon-01 token.
  • I attached a spec of the spacing between buttons and uploaded files, we can follow this for the default file uploader and the drag and drop one.
    Screen Shot 2020-01-27 at 3 43 36 PM

@abbeyhrt
Copy link
Contributor Author

@laurenmrice it should be good for a second look! 👍

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.

React and Vanilla

Drag and drop

  • The first file that uploads needs to be 16px below the drag and drop box. The spacing between multiple files looks good though.
  • The invalid state needs to now be bumped to 48px from 40px. The error border looks inset and the rules are coming outside of it. The border needs to be 2px. Here is a spec of the invalid state since we are bumping this to 48px.

Screen Shot 2020-01-29 at 2 14 23 PM

File uploader

  • The invalid state should follow the above spec too.
  • This isn’t currently in vanilla but we can end up adding it in another pr if you want !

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.

One last thing and we should be good !!

Drag and drop

In React: There needs to be a 16px space below the drag and drop container and the first uploaded file. I think right now its around 20px.

In Vanilla: Needs to be a 16px space below the drag and drop container and the first uploaded file. I think right now its around 8px.

Screen Shot 2020-01-30 at 1 55 31 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.

looks great !!! thank you 🙌🏻

@asudoh
Copy link
Contributor

asudoh commented Jan 30, 2020

@emyarod Do you think this is good to go...? Thanks! And thanks @abbeyhrt for jumping in!

Copy link
Member

@emyarod emyarod 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 to me

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.

[File uploader] Several bugs in style & behavior
6 participants