-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(react): iconButtons have reiterate classNames #15626
fix(react): iconButtons have reiterate classNames #15626
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Sorry I have to update this PR, I will let you know if it's ready! Thank you! |
I updated the PR, so it's ready! Thank you for your review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I like the solution to fix the circular dependency issue. 🚀
Let's wait for another dev to review it!
Thanks for the review, @guidari! 😄 |
…tto' of https://github.com/kuri-sun/carbon into fix/react/iconbuttons-have-reiterate-classnames-in-a-butto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at this! This change will impact Storybook's ability to autogenerate the component API tables.
Right now the defaults for Button are not coming through: https://deploy-preview-15626--v11-carbon-react.netlify.app/?path=/docs/components-button--overview#component-api
We won't want to add BaseButton to the table because it's not exported. I'm not sure how best to fix this without specifying the default for every prop in the storybook args/argTypes.
Sorry everyone! I had a long break so I didn't have time to look at this one! |
Hi @tay1orjones!, Here I attached the preview of this branch's Button components' props table. Please take a look. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, @tay1orjones are you still seeing an issue with prop table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't realize the defaults were broken in prod too. We can fix them in a different issue/PR. Thanks!
e4626be
Hey there! v11.54.0 was just released that references this issue/PR. |
…em#15626) * fix(react): iconButtons have reiterate classNames * fix(react): wrong alignment with button icon only and danger--ghost kind * fix(react): iconButtons have reiterate className --------- Co-authored-by: TJ Egan <[email protected]> Co-authored-by: Andrea N. Cardona <[email protected]>
Closes #15459
Icon buttons have duplicate classNames in their button HTML element because of the circular dependency.
Changelog
New
Changed
Testing / Reviewing
To avoid the circular dependency, introduced a new ButtonBase.
[OLD]
Button > IconButton > Button <--- This is the cause of "circular dependency".
[NEW]
Button > IconButton > ButtonBase
In IconButton, we used to import the Button component once again since we needed to rely on the logic(i.e. generating some classNames, ...etc.) that resides in Button.tsx.
However, this logic in Button.tsx is seperatable. Upon investigation, I noticed that the logic was a mixture of the logic for the Button component(not
hasIconOnly
) and the IconButton component. So, by splitting this logic up and creating a new ButtonBase component to migrate the Button component's logic from Button.tsx.Here is a brief explanation for this.
Button.tsx … If-else condition for deciding whether the Button component or IconButton component.
ButtonBase.tsx … The Button component's logic exists
IconButton.tsx … Export the IconButton component
So in a nutshell, Extracted a logic for the Button component into ButtonBase.tsx from Button.tsx to leave Button.tsx file's responsibility only deciding whether the Button component or IconButton component.
As a pic below, we successfully removed duplicated classNames from the Button component. Also, the circular dependency during the build as well!(#14399)