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(button): center icon inside danger ghost icon only buttons #16923

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

AliAldobyan
Copy link

Closes #16841

The icon inside the IconButton is not center aligned when the danger--ghost is used with kind prop

Changelog

Removed

  • removed "padding-inline-end" in _button.scss file line 286

Testing / Reviewing

1- Go to react storybook.
2- Click on Button Component.
3- Choose Icon Button.
4- Choose danger--ghost as the button kind.

Recording is attached.

Screen.Recording.2024-07-08.at.3.41.46.PM.mov

@AliAldobyan AliAldobyan requested a review from a team as a code owner July 8, 2024 16:04
Copy link
Contributor

github-actions bot commented Jul 8, 2024

All contributors have signed the DCO.
Posted by the DCO Assistant Lite bot.

Copy link

netlify bot commented Jul 8, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit a013e60
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66edc7f9faa0520008319fbe
😎 Deploy Preview https://deploy-preview-16923--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 8, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit a013e60
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66edc7f914ff4d0007d43bad
😎 Deploy Preview https://deploy-preview-16923--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AliAldobyan
Copy link
Author

I have read the DCO document and I hereby sign the DCO.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Hey thanks for taking a look at this!

Removing this style causes the button to inherit the padding styling from the general button styles, which does not match the spec. Ghost buttons should not have additional padding like the other buttons.

This PR:
2024-07-08 at 13 19 57-MAIN-Google Chrome@2x

What's correct in prod right now:
2024-07-08 at 13 19 41-MAIN-Google Chrome@2x

You'll need to target a different selector to apply the proper modifications necessary. It could go after this selector in the icon-only styling block

&.#{$prefix}--btn--ghost .#{$prefix}--btn__icon,
&.#{$prefix}--btn--danger--ghost .#{$prefix}--btn__icon {
margin: 0;
}

@tay1orjones tay1orjones changed the title fix(client): bug 16841 by removing padding inline end fix(button): center icon inside danger ghost icon only buttons Jul 8, 2024
@AliAldobyan
Copy link
Author

AliAldobyan commented Jul 9, 2024

Thank you @tay1orjones for your review, I have applied the modifications by adding

&.#{$prefix}--btn--danger--ghost { padding-inline-end: calc( layout.density('padding-inline') - convert.to-rem(16px) ); }

@AliAldobyan
Copy link
Author

Hello @tay1orjones @alisonjoseph , Can you review the latest update please.

Regards.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

There is a slight upward pixel shift of the vertical icon alignment when comparing with the primary kind

2024-08-12.at.14.20.10-MAIN-Google.Chrome.mp4

@AliAldobyan AliAldobyan force-pushed the fix_bug_the_icon_inside_IconButton_not_aligned_to_center_16841 branch from 8c419f6 to 7fb4c4b Compare August 19, 2024 09:29
@AliAldobyan AliAldobyan force-pushed the fix_bug_the_icon_inside_IconButton_not_aligned_to_center_16841 branch from 7fb4c4b to e0e67fb Compare August 19, 2024 09:31
@AliAldobyan
Copy link
Author

There is a slight upward pixel shift of the vertical icon alignment when comparing with the primary kind

2024-08-12.at.14.20.10-MAIN-Google.Chrome.mp4

There is a slight upward pixel shift of the vertical icon alignment when comparing with the primary kind has been fixed.

Screen.Recording.2024-08-19.at.12.15.23.PM.mov

Copy link
Member

@tay1orjones tay1orjones 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! Sorry for the delay in review

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.

[Bug]: the icon inside IconButton with kind prop danger--ghost is not aligned to center
2 participants