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

feat(icon-button): add padding interface to component FE-5567 #5665

Merged
merged 3 commits into from
Dec 14, 2022
Merged

feat(icon-button): add padding interface to component FE-5567 #5665

merged 3 commits into from
Dec 14, 2022

Conversation

edleeks87
Copy link
Contributor

fix #5640

Proposed behaviour

The IconButton now supports the padding interface to allow consuming projects to override the
default padding values.

No longer sets the padding styling on the internal anchor or button elements rendered internally by
the MenuItem if both onClick and href props are not passed

image

Current behaviour

No support for padding interface in IconButton
Default padding is applied to anchor/ button elements when no onClick/href passed

image

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required

QA

  • Tested in CodeSandbox/storybook
  • Add new Cypress test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

@edleeks87 edleeks87 self-assigned this Dec 1, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 1, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2bce6b7:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart-typescript Configuration
carbon-quickstart PR
elastic-blackwell-zm4ts8 Issue #5640

@cypress
Copy link

cypress bot commented Dec 1, 2022 •
edited
Loading


Test summary

4027 • 0 • 6 • 0 • Flakiness 0

Run details
Project carbon
Status Passed
Commit 45195c6
Started Dec 7, 2022 11:14 AM
Ended Dec 7, 2022 11:20 AM
Duration 06:10 💡
OS Linux Debian - 11.4
Browser Chrome 106

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Parsium Parsium self-requested a review December 6, 2022 10:03
Parsium
Parsium reviewed Dec 6, 2022
View reviewed changes
Copy link
Contributor

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

Happy to approve once the issue @robinzigmond has been addressed 👍🏼 I also noticed the behaviour in the codesandbox where the icon goes dark when the icon button is in focus.

@edleeks87
feat(icon-button): add padding interface to component
2aba921 
The `IconButton` now supports the `padding` interface to allow consuming projects to override the
default padding values.
@edleeks87 edleeks87 force-pushed the FE-5567-padding-menu-item-pop-container branch from 5d67b4d to d960642 Compare December 7, 2022 09:46
robinzigmond
robinzigmond previously approved these changes Dec 7, 2022
View reviewed changes
Parsium
Parsium previously approved these changes Dec 7, 2022
View reviewed changes
@Parsium Parsium marked this pull request as ready for review December 7, 2022 11:01
@Parsium Parsium requested review from a team as code owners December 7, 2022 11:01
@edleeks87
fix(menu-item): prevent default padding on button/anchor inside MenuI…
45195c6 
…tem if no onClick/href set

No longer sets the padding styling on the internal anchor or button elements rendered internally by
the `MenuItem` if both `onClick` and `href` props are not passed

fix #5640
@edleeks87 edleeks87 dismissed stale reviews from Parsium and robinzigmond via 45195c6 December 7, 2022 11:09
@edleeks87 edleeks87 force-pushed the FE-5567-padding-menu-item-pop-container branch from d960642 to 45195c6 Compare December 7, 2022 11:09
@edleeks87 edleeks87 merged commit 33a9e57 into master Dec 14, 2022
@edleeks87 edleeks87 deleted the FE-5567-padding-menu-item-pop-container branch December 14, 2022 11:59
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 111.20.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZhuoyuJin ZhuoyuJin ZhuoyuJin approved these changes

@robinzigmond robinzigmond robinzigmond approved these changes

@Parsium Parsium Parsium approved these changes

Assignees

@edleeks87 edleeks87

Labels
Passed QA Passed Review Passed UX QA released Squad Front End
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allowing padding overriding for MenuItem and IconButton
6 participants
@edleeks87 @carbonci @Parsium @robinzigmond @ZhuoyuJin @harpalsingh