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.

Sign up for GitHub

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

Jump to bottom

fix(button): correct sizes for icon-only buttons #5195

Merged
merged 8 commits into from
Jan 29, 2020
Merged

fix(button): correct sizes for icon-only buttons #5195

merged 8 commits into from
Jan 29, 2020

Conversation

janhassel
Copy link
Member

@janhassel janhassel commented Jan 28, 2020 •
edited
Loading

Closes #5194

Fixes the sizing for icon-only buttons to make them square regardless of size (default, field, small)

Changelog

Changed

  • padding-left and padding-right on icon-only button specific selectors

Testing / Reviewing

Run react storybook, inspect sizes with dev tools

@janhassel janhassel requested a review from a team as a code owner January 28, 2020 14:13
@ghost ghost requested review from asudoh and dakahn January 28, 2020 14:13
@netlify
Copy link

netlify bot commented Jan 28, 2020 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 6e55e58

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

@netlify
Copy link

netlify bot commented Jan 28, 2020 •
edited
Loading

Deploy preview for carbon-components-react failed.

Built with commit 6e55e58

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

@netlify
Copy link

netlify bot commented Jan 28, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 6e55e58

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

@emyarod emyarod requested review from a team and jeanservaas and removed request for a team January 28, 2020 15:17
emyarod
emyarod reviewed Jan 28, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good to me but just want confirmation from the design team regarding this

@laurenmrice
Copy link
Member

@emyarod yes the icon button at each size we have should always be square.

tw15egan
tw15egan approved these changes Jan 28, 2020
View reviewed changes
Copy link
Member

@tw15egan tw15egan 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, thanks for hopping in and fixing this. 👍 ✅

@emyarod emyarod removed the request for review from jeanservaas January 28, 2020 18:37
emyarod
emyarod approved these changes Jan 28, 2020
View reviewed changes
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 pending design approval

laurenmrice
laurenmrice requested changes Jan 28, 2020
View reviewed changes
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 tertiary icon only button (field) in Vanilla needs to be square still.
Screen Shot 2020-01-28 at 12 54 36 PM

Also I noticed some two other things which can be fixed separately in different prs.

  • We don't have a tertiary button in the react storybook and need to add it.
  • Vanilla needs to have the icon only button for all sizes, not just field.

@emyarod emyarod mentioned this pull request Jan 28, 2020
Closed
2 tasks
@emyarod
Copy link
Member

emyarod commented Jan 28, 2020

  • We don't have a tertiary button in the react storybook and need to add it.
  • Vanilla needs to have the icon only button for all sizes, not just field.

these will be addressed in #5201 so we can just update this PR to fix the tertiary icon button

@janhassel
Copy link
Member Author

Good catch, @laurenmrice!
Seems like the different border width (1px instead of 3px) was the cause of that issue. I increased the padding on tertiary icon-only buttons to account for the "missing" 2px on each side.

laurenmrice
laurenmrice approved these changes Jan 29, 2020
View reviewed changes
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 good to me ! thanks for doing this ! 🙌🏻

@asudoh asudoh merged commit 7e83aa7 into carbon-design-system:master Jan 29, 2020
@joshblack joshblack mentioned this pull request Jan 31, 2020
Closed
@joshblack joshblack mentioned this pull request Feb 25, 2020
Closed
@janhassel janhassel deleted the icon-button-fix branch April 2, 2020 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@tw15egan tw15egan tw15egan approved these changes

@dakahn dakahn dakahn approved these changes

@laurenmrice laurenmrice laurenmrice approved these changes

@asudoh asudoh Awaiting requested review from asudoh

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Button]: Icon-only variant has wrong sizes
6 participants
@janhassel @laurenmrice @emyarod @tw15egan @dakahn @asudoh