feat(minor-button): add position: absolute property to icon when component is icon only #5971
Conversation
|
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 a9923bf:
|
Passing run #15934 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
tomdavies73
changed the title
| @@ -17,6 +17,16 @@ function makeColors(color: string) { | |||
| } | |||
|
|
|||
| const StyledButtonMinor = styled(Button)` | |||
| ${({ iconType, iconTooltipMessage, children }) => | |||
| iconType && | |||
| iconTooltipMessage && | |||
There was a problem hiding this comment.
I don't think we need all these conditions.
I see that we need to check for an icon-only button (with the !children check), otherwise the icon overlaps the text when both are present. But iconTooltipMessage makes no difference here (as far as I can tell), and iconType isn't needed simply because without that there's no StyledIcon inside for the styles to affect.
I prefer it checking just this one condition as it makes the code simpler with fewer special cases.
| @@ -88,6 +89,19 @@ describe("Button Minor", () => { | |||
| } | |||
| ); | |||
|
|
|||
| it("when icon only with tooltip message, icon's position is absolute", () => { | |||
| const wrapper = mount( | |||
| <ButtonMinor iconType="bin" iconTooltipMessage="foo" /> | |||
There was a problem hiding this comment.
If you make the code change I suggested,the iconTooltipMessage prop won't be needed in this prop (and we can leave it out of the test description).
| @@ -94,6 +94,13 @@ context("Test for Button Minor component", () => { | |||
| } | |||
| ); | |||
|
|
|||
| it("when icon only with tooltip message, icon's position is absolute", () => { | |||
| CypressMountWithProviders( | |||
| <ButtonMinor iconType="bin" iconTooltipMessage="foo" /> | |||
There was a problem hiding this comment.
If you make the code change I suggested,the iconTooltipMessage prop won't be needed in this prop (and we can leave it out of the test description).
tomdavies73
changed the title
tomdavies73
force-pushed
the
FE-5742
branch
2 times, most recently
from
96f2230 to
2aa4d5a
Compare
| const wrapper = mount(<ButtonMinor iconType="bin">Foo </ButtonMinor>); | ||
| assertStyleMatch( | ||
| { | ||
| position: "undefined", |
There was a problem hiding this comment.
| position: "undefined", | |
| position: undefined, |
I'm surprised your version works, I guess all values get coerced to a string somewhere in assertStyleMatch. But we don't want the CSS position value to be "undefined", we want it to be not there (corresponding to JS undefined, which isn't a string).
…onent is icon only adds position: absolute CSS property to `Icon` when component is icon only. This prevents the rendered `Icon` from moving position on hover due to a reduction in padding added during `ButtonMinor`s composition, which causes the `Icon` and Portal used to to provide a tooltip message to stack on top of one another fix #5946
|
🎉 This PR is included in version 118.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix #5946
Proposed behaviour
This PR will add position: absolute CSS property to
Iconwhen component is icon only. This prevents the renderedIconfrom moving position on hover due to a reduction in padding added duringButtonMinors composition, which causes theIconand Portal used to to provide a tooltip message to stack on top of one anotherCurrent behaviour
Currently, the
Iconrendered within the component and Portal used to to provide a tooltip message stack on top of one another, this is due to the rendredIconhaving relative positioning, which is needed for other variants of the button and a reduction in padding implemented during the components composition.Checklist
d.tsfile added or updated if requiredQA
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].