Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

fix(button): follow core Carbon icon positioning #697

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Sep 3, 2021

Related Ticket(s)

#696

Description

This PR updates the button styles to more closely match the styles from core Carbon and apply them to the web component version. Confirm that all button variants (default, icon, text + icon) appear as expected (including expressive versions)

image

original:

image

updated:

image

core Carbon (expressive):

image

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 3, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 3, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 3, 2021

@oliviaflory
Copy link

@emyarod I noticed the ghost and ghost danger buttons have a slight issue where the icon and text overlap in the text + icon variants:
Screen Shot 2021-09-03 at 10 00 43 AM
Screen Shot 2021-09-03 at 10 03 41 AM

Otherwise looks great!

@emyarod emyarod requested a review from oliviaflory September 3, 2021 17:24
@emyarod
Copy link
Member Author

emyarod commented Sep 3, 2021

@oliviaflory the ghost variants should be fixed now, also added a fix for the condensed icon layout!

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 3, 2021

@oliviaflory
Copy link

@emyarod thanks for correcting the overlap!

My one question is about the condensed versions of the ghost buttons

In the primary button, the regular is spaced out with ~64px between the text and icon, and the condensed seems to be ~ 4 or 8px space between text and icon

Regular
Screen Shot 2021-09-08 at 11 44 30 AM
Condensed
Screen Shot 2021-09-08 at 11 43 08 AM

In the ghost button version, they seem to be the same amount of space

Regular
Screen Shot 2021-09-08 at 11 44 18 AM

Condensed
Screen Shot 2021-09-08 at 11 43 59 AM

I can't see any knobs to change from regular to condensed in the Carbon react storybook, so I'm not sure what the expected spacing is, would you mind double checking? If this is the same as the Carbon one, lmk and I can approve!

@emyarod
Copy link
Member Author

emyarod commented Sep 8, 2021

@oliviaflory the condensed sizing seems to be exclusive to carbon-web-components, it was introduced here (#468) for better support for the data table's action bar in core Carbon. If this is something we don't want to display in our storybook I can remove that option

alternatively I can make the condensed ghost button match the other condensed buttons. which option would you prefer to go with?

@shixiedesign
Copy link

Hey @emyarod @oliviaflory , looking into this a bit, it seems like Carbon react handles ghost button styles differently than Carbon web components. Is it simply that WC's ghost button styling need an update? Ideally we don't introduce new variables .

Sep-08-2021 13-58-47

Sep-08-2021 13-59-38

@emyarod
Copy link
Member Author

emyarod commented Sep 8, 2021

@shixiedesign @oliviaflory sounds like it would be better to remove the condensed option from the storybook and keep it internal to WC then. we can also separate the work for aligning the ghost button implementations from this PR so that the downstream expressive modal work can continue in the meantime

@emyarod
Copy link
Member Author

emyarod commented Sep 9, 2021

@oliviaflory @shixiedesign the ghost buttons should be resolved now. I left the icon-layout knob in Storybook, but it's specific to carbon-web-components. it just has no effect on the ghost buttons (but will have effects on the other button types)

Copy link
Member

@jeffchew jeffchew left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@oliviaflory oliviaflory left a comment

Choose a reason for hiding this comment

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

thanks @emyarod 👍

@emyarod emyarod added Ready to merge Label for the pull requests that are ready to merge and removed Needs design approval 👀 eyes needed labels Sep 13, 2021
@kodiakhq kodiakhq bot merged commit db4e4c1 into carbon-design-system:master Sep 13, 2021
@emyarod emyarod deleted the 696-fix/button-style-discrepancies branch September 14, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants