-
Notifications
You must be signed in to change notification settings - Fork 43
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
Set Card content max-width #943
Conversation
🦋 Changeset detectedLatest commit: bdb51e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
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.
@danielguillan it looks like the overall width of the card has been affected here somehow? Can't immediately see how that would happen though, any ideas? I wouldn't expect this diff here.
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.
@rezrah Good catch! With this update, the card's maximum total width is now limited to 400px
(based on the descriptions' max-width
) instead of the 420px
max-width
set at the container level.
To avoid regressions, we could match the description's max-width
to the cards' current max-width
(i.e., 420px
). Wdyt? We could set 400px
on the full-width variant only, but I don't believe it's worth it.
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.
That sounds like a good plan 👍. Yeah let's do that please. If we can limit scope of these changes to inner content only like the PR indicates, that would help reduce the testing burden and risks of regressions outside of the card.
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
One question for you @danielguillan, which looks like it could either be a regression or flakey test? Hopefully latter, as otherwise this could cause significant downstream regression too 🤔
36d50e1
to
6d53b8f
Compare
6d53b8f
to
bdb51e0
Compare
@rezrah I limited these changes to the full-width variant to avoid any potential regressions in card instances with an intrinsic size. The default variant, which has a maximum width at the container level, already constrains the content length. |
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.
🚀
Summary
Sets a maximum width for the heading and description of the
Card
component to prevent very long lines on wider cards.Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots: