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): prevent icon and tooltip jumping for tertiary button #5618

Merged
merged 8 commits into from
Mar 16, 2020
Merged

fix(button): prevent icon and tooltip jumping for tertiary button #5618

merged 8 commits into from
Mar 16, 2020

Conversation

janhassel
Copy link
Member

@janhassel janhassel commented Mar 13, 2020 •
edited
Loading

Closes #5617

Simplifies the button styling and fixes the following problems:

  1. Icon in tertiary button jumps when focused (Button: icon in tertiary button jumps on focus #5617)
  2. Tooltip jumps when focusing icon-only tertiary button
  3. Border color of danger button changes color on hover when focused

(see screen recordings posted below)

Changelog

Changed

  • All buttons use the same border / box-shadow setup to render focused styles now, eliminating a lot of special declarations for the tertiary button
  • Deprecated unused $button-outline-offset and $button-outline tokens

Removed

  • $border-width parameter of button-theme mixin

Testing / Reviewing

  1. Open react storybook -> Buttons
  2. Re-create the scenario in the screen recordings posted below
  3. Compare to behaviour in current react storybook

Additional resources

  1. Icon in tertiary button jumps when focused (Button: icon in tertiary button jumps on focus #5617)
  • Screen Recording 2020-03-13 at 14 13 49
  1. Tooltip jumps when focusing icon-only tertiary button
  • Screen Recording 2020-03-13 at 14 10 06
  1. Border color of danger button changes color on hover when focused
  • Screen Recording 2020-03-13 at 14 11 34

@janhassel janhassel requested a review from a team as a code owner March 13, 2020 11:10
@ghost ghost requested review from abbeyhrt and aledavila March 13, 2020 11:10
@netlify
Copy link

netlify bot commented Mar 13, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit f23f2e8

https://deploy-preview-5618--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Mar 13, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit f23f2e8

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

@janhassel
fix(button): simplify button stylings
8ec8964 
This addresses the following problems:
- jumping tooltip on icon-only tertiary button
- red border color for focused danger button when hovering
@janhassel janhassel changed the title fix(button): prevent jump of icon in tertiary button fix(button): prevent icon and tooltip jumping for tertiary button Mar 13, 2020
@emyarod emyarod requested a review from laurenmrice March 13, 2020 13:39
emyarod
emyarod reviewed Mar 13, 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.

on a side note, if users are using the tokens I think we may need to deprecate them rather than removing them outright before a major release

@janhassel
Copy link
Member Author

@emyarod True, didn't think about that. Reverted removal and added /// @deprecated note to the tokens.

laurenmrice
laurenmrice requested changes Mar 13, 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.

Everything you noted is working correctly now 👍🏻

One other thing I am noticing is that on active the danger button is getting some kind of lighter red border around it. That should not be showing up.
Mar-13-2020 13-16-36

@tw15egan
Copy link
Collaborator

Looks like this will also close out #5263

@tw15egan tw15egan mentioned this pull request Mar 13, 2020
Closed
tw15egan
tw15egan reviewed Mar 13, 2020
View reviewed changes
Copy link
Collaborator

@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.

Seeing an issue with icon only buttons + svg fill color

icon-primary

@janhassel
Copy link
Member Author

@laurenmrice Thanks for catching that. Should be fixed now.

@tw15egan Weird. That didn't show up in my local setup. I think I know where the issue lies though and tried to fix it. We'll see once the deploy preview is done building.
Regarding the issue about the 2px border you mentioned: I cannot really see a difference between this implementation and the current 10.10.1 one. Should I adjust anything?

@tw15egan
Copy link
Collaborator

tw15egan commented Mar 13, 2020 •
edited
Loading

@janhassel I just noticed that you modified the border token width, which is all that the issue was talking about 👍

https://github.com/carbon-design-system/carbon/pull/5618/files#diff-cfdf7d6a6baff15d13476bc700c119b9R134

Other issues look resolved now!

laurenmrice
laurenmrice approved these changes Mar 13, 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 great, thank you ! 🙌🏻

@asudoh
Copy link
Contributor

asudoh commented Mar 14, 2020

@emyarod Any further comments...? Thanks!

emyarod
emyarod approved these changes Mar 16, 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

@joshblack joshblack merged commit e62f8d9 into carbon-design-system:master Mar 16, 2020
aledavila pushed a commit that referenced this pull request Mar 18, 2020
@janhassel @tw15egan @joshblack
fix(button): prevent icon and tooltip jumping for tertiary button (#5618
f32c1a6 
)

* fix(button): prevent jump of icon in tertiary button

* fix(button): simplify button stylings

This addresses the following problems:
- jumping tooltip on icon-only tertiary button
- red border color for focused danger button when hovering

* fix(button): correct box-shadows

* fix: deprecate unused button-outline-offset and button-outline tokens

* fix(button): correct danger button hover and icon-only svg fill

Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: Josh Black <[email protected]>
This was referenced Mar 18, 2020
Closed
Closed
renmaddox pushed a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
@janhassel @tw15egan
@joshblack @renmaddox
fix(button): prevent icon and tooltip jumping for tertiary button (ca…
734a393 
…rbon-design-system#5618)

* fix(button): prevent jump of icon in tertiary button

* fix(button): simplify button stylings

This addresses the following problems:
- jumping tooltip on icon-only tertiary button
- red border color for focused danger button when hovering

* fix(button): correct box-shadows

* fix: deprecate unused button-outline-offset and button-outline tokens

* fix(button): correct danger button hover and icon-only svg fill

Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: Josh Black <[email protected]>
renmaddox added a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
@renmaddox
Revert "fix(button): prevent icon and tooltip jumping for tertiary bu…
6228169 
…tton (carbon-design-system#5618)"

This reverts commit 734a393.
renmaddox added a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
@renmaddox
Revert "fix(button): prevent icon and tooltip jumping for tertiary bu…
da594c1 
…tton (carbon-design-system#5618)"

This reverts commit 734a393.
renmaddox pushed a commit to renmaddox/carbon that referenced this pull request Mar 20, 2020
@janhassel @tw15egan
@joshblack @renmaddox
fix(button): prevent icon and tooltip jumping for tertiary button (ca…
a6d4973 
…rbon-design-system#5618)

* fix(button): prevent jump of icon in tertiary button

* fix(button): simplify button stylings

This addresses the following problems:
- jumping tooltip on icon-only tertiary button
- red border color for focused danger button when hovering

* fix(button): correct box-shadows

* fix: deprecate unused button-outline-offset and button-outline tokens

* fix(button): correct danger button hover and icon-only svg fill

Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: Josh Black <[email protected]>
@joshblack joshblack mentioned this pull request Mar 31, 2020
Closed
@janhassel janhassel deleted the fix-5617 branch April 2, 2020 15:48
@asudoh asudoh mentioned this pull request Apr 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@tw15egan tw15egan tw15egan approved these changes

@emyarod emyarod emyarod approved these changes

@aledavila aledavila Awaiting requested review from aledavila

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 in tertiary button jumps on focus
7 participants
@janhassel @tw15egan @asudoh @emyarod @abbeyhrt @laurenmrice @joshblack