Don't apply flash* classes to banners #2289
Conversation
🦋 Changeset detectedLatest commit: 6034d2e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
One downside to this approach is it'll increase the specificity of the flash styles, if there's any custom CSS in dotcom overriding one of the properties here it'll stop working. We chatted about this and decided that we could address those places specifically by increasing the importance or refactoring away.
I had a non-blocking question but overall think this should be safe to do.
src/alerts/flash.scss
Outdated
| @@ -86,7 +86,7 @@ | |||
| } | |||
| } | |||
|
|
|||
| .flash-warn { | |||
| .flash-warn:not(.Banner--warning) { | |||
There was a problem hiding this comment.
It maybe sufficient enough here to include :not(.Banner) on all of these instead of the scheme but could also be fine to include this as the Banner scheme also if you think there's a case where someone would have flash-* with Banner--* but not have Banner
There was a problem hiding this comment.
Hmm interesting. I don't think that would happen? Let's go with :not(.Banner) for now.
What are you trying to accomplish?
We've run into some issues attempting to migrate usages of
Primer::Beta::FlashtoPrimer::Beta::Banner, namely that existing CSS and JS in dotcom select on theflashclass and its friends. This has made it really difficult to migrate to theBannercomponent because theflashclass is no longer used.What approach did you choose and why?
To ease the transition, we propose adding these legacy class names to banner elements while updating primer/css to only apply flash styles to elements that are not also banner elements.
Can these changes ship as is?