-
Notifications
You must be signed in to change notification settings - Fork 84
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(link): ensure Icon in component has inline display property to respect dimensions of the parent #6349
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 5b0d61e:
|
b215da6
to
03cbe15
Compare
@@ -72,6 +73,18 @@ context("Test for Link component", () => { | |||
tooltipPreview().should("have.text", testCypress); | |||
}); | |||
|
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.
question: wondering if these cypress tests would be necessary here, given we are testing the same styling with jest tests?
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.
Good question, technically we could probably get away with just having unit tests, but I thought it was worth just doubling up as we've done the same for other features in Link
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.
Don't really mind if you leave both unit and CI tests in tbh. However I do think a test story so chromatic can capture it would be best
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.
Nothing much beyond what @Parsium has commented already
parameters: { | ||
info: { disable: true }, | ||
chromatic: { | ||
disableSnapshot: true, | ||
disableSnapshot: false, |
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 will mean chromatic captures both the DefaultStory
and FlexContainer
stories. If that's not what you intended I think revert this and set the parameters directly on the FlexContainer story.
FlexContainer.parameters = { chromatic: { disableSnapshot: false }};
…spect dimensions of the parent all children of anchor elements in `Link` have the inline-block display property applied. However due to our new `Icon` font sizes this now causes `Icon`s to not respect the dimensions of parent containers. To mitigate this we've set display to inline instead when `Link`s are rendered with no children and an `Icon`, ensuring the parents dimensions are properly respected fix #6295
🎉 This PR is included in version 122.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix #6295
Proposed behaviour
All elements which are children of anchor elements in
Link
have theinline-block
display property applied. However, due to our newIcon
font sizes this now causesIcon
s to not respect the dimensions of parent containers.To mitigate this we've set display to
inline
instead whenLink
s are rendered with no children and anIcon
, ensuring the parents dimensions are properly respected.Current behaviour
All elements which are children of anchor elements in
Link
have theinline-block
display property applied, causing a visual bug whereIcon
s do not properly respect the dimensions of their parent containersChecklist
d.ts
file 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]
.