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): update icon only ghost button colors #5313

Merged
merged 5 commits into from
Feb 17, 2020
Merged

fix(button): update icon only ghost button colors #5313

merged 5 commits into from
Feb 17, 2020

Conversation

tw15egan
Copy link
Collaborator

Closes #5312

Sets icon fill to icon-01 when using icon only ghost button

Changelog

New

  • Adds icon only ghost button back into react storybook
  • Fill is now icon-01

Testing / Reviewing

Ensure icon only ghost button renders with icon-01

@tw15egan tw15egan requested a review from a team as a code owner February 10, 2020 21:57
@ghost ghost requested review from abbeyhrt and dakahn February 10, 2020 21:57
@netlify
Copy link

netlify bot commented Feb 10, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 97faa2f

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

@netlify
Copy link

netlify bot commented Feb 10, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 97faa2f

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

laurenmrice
laurenmrice requested changes Feb 10, 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.

For the disabled state, the icon needs to be disabled-02. I also think they should not get a tooltip when disabled and should get the disabled cursor change? (@dakahn may be able to confirm that though)

Can we have a way where we can switch between viewing it as icon-01 (being the default) and icon-02. Or is this something developers can change easily on their own end? I say this because the icon-02 is a frequent usecase when there are multiple of these icons in a row or near eachother.

@tw15egan
Copy link
Collaborator Author

Right now none of our button variations have specific props that can change color, but I guess we can add one. Is this similar to the light variations of inputs? Just unsure of the use case / why we need to have multiple icon colors for this variation, but none of our others have this feature.

@laurenmrice
Copy link
Member

laurenmrice commented Feb 11, 2020 •
edited
Loading

It wouldn't be similar to the light prop. Is this something devs can easily change if they wanted to on their own? If so we can just keep the icon-01 and note in our doc guidance when to use icon-01 versus icon-02. People use this type of button differently within their UIs than other types of buttons (ex: as action icons on the page next to certain components, adding or deleting things, etc) and there are usecases for both.

@tw15egan
Copy link
Collaborator Author

Yeah they can always just set the icon to $icon-02 via their own css if needed

@laurenmrice
Copy link
Member

Ok cool, then how you have it is good. 👍🏻 Just need to update the disabled state icon to be disabled-02. It should not get a tooltip and should get the disabled cursor change instead.

@tw15egan tw15egan force-pushed the icon-only-ghost-button branch from ac92739 to f8e77ad Compare February 11, 2020 16:28
@tw15egan
Copy link
Collaborator Author

@laurenmrice is this good to go?

laurenmrice
laurenmrice requested changes Feb 11, 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.

There shouldn't be a background on the disabled state. only the icon needs to be disabled-02

@tw15egan
Copy link
Collaborator Author

Does the spec here (#5312) need to be updated then?

@laurenmrice
Copy link
Member

laurenmrice commented Feb 11, 2020 •
edited
Loading

oh sorry! yes, i just updated it to reflect that change.

@tw15egan tw15egan force-pushed the icon-only-ghost-button branch from 177e6e9 to 44e9e5f Compare February 11, 2020 23:48
@tw15egan
Copy link
Collaborator Author

@laurenmrice updated 👍

laurenmrice
laurenmrice approved these changes Feb 12, 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.

awesome ! looks good to me 🙌🏻

@tw15egan tw15egan merged commit 8adc849 into carbon-design-system:master Feb 17, 2020
@aagonzales aagonzales mentioned this pull request Feb 18, 2020
Closed
2 tasks
@joshblack joshblack mentioned this pull request Feb 25, 2020
Closed
@laurenmrice laurenmrice mentioned this pull request Mar 26, 2020
Closed
@zJag zJag mentioned this pull request Mar 27, 2020
Closed
@tw15egan tw15egan mentioned this pull request Mar 31, 2020
Merged
@emyarod emyarod mentioned this pull request Jan 22, 2021
Closed
@tw15egan tw15egan deleted the icon-only-ghost-button branch April 28, 2021 18:17
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

@dakahn dakahn dakahn approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@joshblack joshblack joshblack approved these changes

@emyarod emyarod Awaiting requested review from emyarod

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.

[icon button] add ghost icon button functionality to react
6 participants
@tw15egan @laurenmrice @joshblack @abbeyhrt @dakahn @emyarod