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! |
|
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.
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.
This LGTM, @tay1orjones are you still seeing an issue with prop table?
tay1orjones
added
status: ready to merge 🎉
and removed
status: one more review 👀
labels
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 <tw15egan@gmail.com> Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
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)
