-
Notifications
You must be signed in to change notification settings - Fork 42
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
Alert: Misc. adjustments #3007
Alert: Misc. adjustments #3007
Conversation
🦋 Changeset detectedLatest commit: 779a514 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
Storybook demoEndringer til review: 1ec5b49e45 | 85 komponenter | 160 stories |
as="div" | ||
size={size} | ||
className="navds-alert__wrapper" | ||
style={contentMaxWidth ? { maxWidth: "43.5rem" } : undefined} |
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 change will prob break some applications since inline-styles are the most specific when it comes to CSS-overrides. Maybe something like this could be non-breaking
className={cl("navds-alert__wrapper", {"navds-alert__wrapper--no-maxwidth": !contentMaxWidth})}
:where(.navds-alert__wrapper:not(.navds-alert__wrapper--no-maxwidth)) {
max-width: 43.5rem;
}
Since :where
is used it will have 0 specificity even when using not
(not tested here so might not be 100% correct)
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.
I agree that inline style was a bad idea 😅 But do we need :where
? Doesn't custom css always come after ds-css
?
.navds-alert__wrapper--maxwidth {
max-width: 43.5rem;
}
.navds-alert__wrapper {
max-width: 100px; /* <-- works */
}
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.
No could probably simplify it 👍 As long as targeting navds-alert__wrapper
still allows those who have overwritten it now won't notice any changes.
https://github.com/navikt/team-aksel/issues/491