[Banner] Consolidate se23 logic and styles #10001
Conversation
sophschneider
force-pushed
the
sophie/banner-cleanup
branch
3 times, most recently
from
c3b7e2d to
a6a4f99
Compare
sophschneider
requested review from
gwyneplaine,
jesstelford,
kyledurand,
laurkim and
sam-b-rose
sophschneider
force-pushed
the
sophie/banner-cleanup
branch
from
a6a4f99 to
56a39aa
Compare
| icon: InfoMinor, | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Moved this utility over from the old Banner.tsx file (it used to be at the end of it)
| WithinContentContainerBanner, | ||
| } from '../components/BannerExperimental/BannerExperimental'; | ||
|
|
||
| describe('<Banner />', () => { |
There was a problem hiding this comment.
These were the tests for the old Banner, the tests for the new one are in a different describe block below
| @@ -1 +1,2 @@ | |||
| export * from './Banner'; | |||
| export type {BannerHandles} from './utilities'; | |||
There was a problem hiding this comment.
This was previously part of the * export in Banner, but since I moved it to utilities, I have to export it separately
| </BannerContext.Provider> | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The code below was moved from BannerExperimental
| props: BannerProps, | ||
| bannerRef, | ||
| ) { | ||
| const { |
There was a problem hiding this comment.
Removed all old unused tsx from the old Banner
| right: var(--p-space-4); | ||
| top: var(--p-space-5); | ||
| position: absolute; | ||
| margin-top: var(--p-space-4); |
There was a problem hiding this comment.
This should have been --p-space-4 (its the spacing between multiple banners)
| @@ -1,50 +1,9 @@ | |||
| @import '../../styles/common'; | |||
|
|
|||
| @mixin banner-variants($in-page) { | |||
There was a problem hiding this comment.
Not using this mixin anymore as it just made the file confusing. I put the appropriate styles in the withinPage and withinContentContainer classes since thats the only place this mixin was being called. Shared styles went to .Banner. .Banner, .withinPage, and .withinContentContainer are all only applied to the same wrapping div
| outline: var(--p-border-width-2) solid | ||
| var(--p-color-border-interactive-focus); | ||
| } | ||
|
|
There was a problem hiding this comment.
None of these status class names are applied when the flag is true
| display: flex; | ||
|
|
||
| // stylelint-disable selector-max-class, selector-max-specificity -- generated by polaris-migrator DO NOT COPY | ||
| &.statusCritical .PrimaryAction.Button { |
There was a problem hiding this comment.
None of these classnames are applied when the flag is true
| padding: 0; | ||
| } | ||
|
|
||
| .Dismiss { |
There was a problem hiding this comment.
.Dismiss isn't being applied when the flag is true
sophschneider
force-pushed
the
sophie/banner-cleanup
branch
from
56a39aa to
0ee47ff
Compare
sam-b-rose
force-pushed
the
next
branch
4 times, most recently
from
5d459f8 to
700a72f
Compare
sam-b-rose
force-pushed
the
sophie/banner-cleanup
branch
from
0ee47ff to
8ac1c54
Compare
sam-b-rose
left a comment
sam-b-rose
left a comment
There was a problem hiding this comment.
Legendary clean up 🧹 ✨ looks good
### WHY are these changes introduced? Fixes #9919 ### WHAT is this pull request doing? * Removes se23 conditional logic for `Banner.tsx` * Consolidates se23 conditional styles for `Banner.scss` * Removes unused styles * Merges `BannerExperimental` with main Banner ### How to 🎩 Compare production and this PR's chromatic storybook to make sure styles are the same [Production](https://storybook.polaris.shopify.com/?path=/story/all-components-banner--all&globals=polarisSummerEditions2023:true) [This PR](https://5d559397bae39100201eedc1-cwkvegwhxb.chromatic.com/?path=/story/all-components-banner--all)
### WHY are these changes introduced? Fixes #9919 ### WHAT is this pull request doing? * Removes se23 conditional logic for `Banner.tsx` * Consolidates se23 conditional styles for `Banner.scss` * Removes unused styles * Merges `BannerExperimental` with main Banner ### How to 🎩 Compare production and this PR's chromatic storybook to make sure styles are the same [Production](https://storybook.polaris.shopify.com/?path=/story/all-components-banner--all&globals=polarisSummerEditions2023:true) [This PR](https://5d559397bae39100201eedc1-cwkvegwhxb.chromatic.com/?path=/story/all-components-banner--all)
### WHY are these changes introduced? Fixes Shopify#9919 ### WHAT is this pull request doing? * Removes se23 conditional logic for `Banner.tsx` * Consolidates se23 conditional styles for `Banner.scss` * Removes unused styles * Merges `BannerExperimental` with main Banner ### How to 🎩 Compare production and this PR's chromatic storybook to make sure styles are the same [Production](https://storybook.polaris.shopify.com/?path=/story/all-components-banner--all&globals=polarisSummerEditions2023:true) [This PR](https://5d559397bae39100201eedc1-cwkvegwhxb.chromatic.com/?path=/story/all-components-banner--all)
2 participants
WHY are these changes introduced?
Fixes #9919
WHAT is this pull request doing?
Banner.tsxBanner.scssBannerExperimentalwith main BannerHow to 🎩
Compare production and this PR's chromatic storybook to make sure styles are the same
Production
This PR