feat(divider): migrate to second-gen styles and architecture #5798
Conversation
|
rise-erpelding
added
2nd gen
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
There was a problem hiding this comment.
Keeping this separate so it can be rolled back easily if we don't want to add it at this time. I noticed that the static color controls on progress circle didn't apply a background making the static white invisible, so this would address that as well.
There was a problem hiding this comment.
This is great! I think we should definitely keep it for now. It is one of those things we can always revisit once we dive deeper into our Storybook setup.
| /* | ||
| * @todo This is properly configuring the Select, but the control doesn't | ||
| * seem to work; need to investigate. | ||
| */ | ||
|
|
||
| // argTypes.size = { | ||
| // ...argTypes.size, | ||
| // control: { type: 'select' }, | ||
| // options: Divider.VALID_SIZES, | ||
| // }; |
There was a problem hiding this comment.
This is the same situation as with Badge and Progress Circle, copying this over from those components.
There was a problem hiding this comment.
Could it be related to us not defining the size property on the component? What I understand from Cursor is that we get the argTypes using getStorybookHelpers, but getStorybookHelpers doesn't bind the size attribute to the component in the "regular" template(args) render we're trying to use.
So because divider extends the SizedMixin(), but we're only pulling the argTypes from divider specifically, is getStorybookHelpers not seeing the size property?
While I was experimenting with this in status light, if I use a different render method that binds the size property:
render: (args) =>
html`<swc-status-light .size=${args.size} variant=${args.variant}
>${args['default-slot']}</swc-status-light
>`,
I think I can get the commented out code to work as we're expecting. Not sure if that's something we want to do right now though.
There was a problem hiding this comment.
@marissahuysentruyt Thank you for digging into this! I think the solution you came up with is a good workaround, and one we may ultimately end up adopting, but I'd like to hold off for now so we can troubleshoot our Custom Element Manifest tooling pipeline and figure out why mixin-defined properties aren't propagating as we'd like them to.
rise-erpelding
changed the title
| } | ||
|
|
||
| /* windows high contrast mode */ | ||
| @media (forced-colors: active) { |
There was a problem hiding this comment.
If you test WHCM or forced-colors (even adjusting the Rendering in Chrome will reveal this), you'll see the divider on the docs page looks something like this:
I did investigate this, and it looks like it's coming from Storybook styling that's adding an 8px transparent border that shows up in forced colors. We might want to remove that eventually, but that feels a little out of scope for now. And the border isn't visible when you view the default story.
There was a problem hiding this comment.
Hmm, is this something we can override with forced specificity in the Storybook styles?
There was a problem hiding this comment.
Was this what you meant? I added some styles to a new preview-head.html file in f0d7607 but the class selector needs to be tripled in order to increase the specificity enough to override.
| validSizes: DIVIDER_VALID_SIZES, | ||
| noDefaultSize: true, | ||
| }) { | ||
| static readonly STATIC_COLORS = DIVIDER_STATIC_COLORS; |
There was a problem hiding this comment.
We’re still missing the contributing documentation that will clarify these nuances (SWC-1234). However, in this specific case, we don’t need to add this static variable. It is only needed to handle cases where these values differ from S1/S2 and we want to throw dev warnings (e.g., deprecation, invalid value) at runtime. By making these values class properties, we can access them through the constructor and retrieve the correct value being loaded, whether it’s from the first or second generation. Hopefully, we can make this information clear enough in the aforementioned documentation! 😛
There was a problem hiding this comment.
@rise-erpelding: I discussed this briefly offline with @rubencarvalho, and we agreed that for now we'd like to consistently apply the pattern where we hang structured Spectrum data-type arrays off of the component's constructor.
Let's do this even in cases where the data type doesn't vary from S1 to S2, and even in cases where we don't have a specific need for the structured types at runtime.
At minimum, doing this consistently means we should essentially never have to separately import the structured type arrays into a component's stories—we can always just import the constructor and access the type arrays from there.
The fundamental reasoning behind this is to minimize cognitive load for component authors and maximize consistency.
In the case of Divider, this would mean defining static readonly properties on the base component for both VALID_SIZES and STATIC_COLORS.
In the case of Divider, this would mean defining a static readonly property on the base component for STATIC_COLORS. (It's not necessary to define one for VALID_SIZES, since SizedMixin does that for you.)
Let me know if any of this isn't clear!
There was a problem hiding this comment.
This is great! I think we should definitely keep it for now. It is one of those things we can always revisit once we dive deeper into our Storybook setup.
|
Also, that screenshot in the description is :chefkiss: |
| "types": "./src/index.d.ts", | ||
| "dependencies": { | ||
| "@spectrum-web-components/base": "1.8.0", | ||
| "@spectrum-web-components/shared": "1.8.0", |
There was a problem hiding this comment.
I am not sure we need the shared dependency 🤔
| "@spectrum-web-components/shared": "1.8.0", |
| /** | ||
| * Static color background settings - matching spectrum-css gradients | ||
| */ | ||
| const staticColorSettings = { |
There was a problem hiding this comment.
Thanks so much for adding this! After this merges, I could go back into progress circle and update it to use this. 👯
|
|
||
| import type { ElementSize } from '@swc/core/shared/base'; | ||
|
|
||
| export const DIVIDER_VALID_SIZES: ElementSize[] = ['s', 'm', 'l'] as const; |
| protected override render(): TemplateResult { | ||
| return html``; | ||
| return html` | ||
| <div |
| ); | ||
| overflow: visible; | ||
|
|
||
| &:not(&.spectrum-Divider--vertical) { |
There was a problem hiding this comment.
| &:not(&.spectrum-Divider--vertical) { | |
| &:not(.spectrum-Divider--vertical) { |
I don't think the amper used inside the not selector is going to give the result you are looking for here.
There was a problem hiding this comment.
Nice catch! I had copied over my CSS without cleaning it up (thus the not removing of the mods). Do you have a preference on nesting/no nesting, for instance with something like:
.spectrum-Divider--staticWhite {
--spectrum-divider-background-color: var(--spectrum-transparent-white-200);
&.spectrum-Divider--sizeL {
--spectrum-divider-background-color: var(--spectrum-transparent-white-800);
}
}It didn't look like badge and progress circle CSS were using it, so I will be removing it here as well (relying more on what's in the dist index.css), but let me know if that's too much!
| min-block-size: var( | ||
| --mod-divider-block-minimum-size, | ||
| var(--spectrum-divider-block-minimum-size) | ||
| ); |
There was a problem hiding this comment.
| min-block-size: var( | |
| --mod-divider-block-minimum-size, | |
| var(--spectrum-divider-block-minimum-size) | |
| ); | |
| min-block-size: var(--spectrum-divider-block-minimum-size); |
Would you mind going through here and cleaning up the mods? Since we're not supporting mods in S2, the idea was to remove them in Spectrum CSS before we bring over the code but we haven't had a chance to do that yet.
| } | ||
|
|
||
| /* windows high contrast mode */ | ||
| @media (forced-colors: active) { |
There was a problem hiding this comment.
Hmm, is this something we can override with forced specificity in the Storybook styles?
| handles: events, | ||
| }, | ||
| }, | ||
| tags: ['migrated'], |
There was a problem hiding this comment.
Good work. Some observations on few things like
- Type separation, double check the removed
hrtag if that is intentional? - Can you also add a runtime validation for user-facing props like if the user passes invalid value for
sizeorstatic-colordoes the component fail gracefully?
| export const DIVIDER_VALID_SIZES: ElementSize[] = ['s', 'm', 'l'] as const; | ||
| export const DIVIDER_STATIC_COLORS = ['white', 'black'] as const; |
There was a problem hiding this comment.
How do you feel separating the types and the runtime values in .consts.ts and .types.ts files?
There was a problem hiding this comment.
Done! 1c40f78
There was a problem hiding this comment.
@Rajdeepc @rise-erpelding While I'm definitely open to evolving in this direction as we work our way through more components and refine our approach to defining structured Spectrum data types, I'd prefer to keep it simple for now by maintaining just one file.
My reasoning here is that I could see us evolving in various directions from this initial starting point, so doing this refactor now seems premature.
| class=${classMap({ | ||
| ['spectrum-Divider']: true, | ||
| [`spectrum-Divider--size${this.size?.toUpperCase()}`]: | ||
| typeof this.size !== 'undefined', |
There was a problem hiding this comment.
You can also use this.size != null for both null and undefined.
| hr { | ||
| border: none; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
Can you please double check this is not a breaking change?
There was a problem hiding this comment.
I don't see this component rendering an <hr>, it renders an empty template for first-gen (and then our div wrapper with the classes on it for second-gen), so it doesn't seem like those styles were doing anything there, unless I'm missing something.
I will say that I see the CSS implementation optionally using <hr> though (and while the styles do set border: none, the margin isn't corrected for the horizontal divider), so I'm not sure if SWC once used <hr> as well and this is remaining from that.
rise-erpelding
force-pushed
the
rise-erpelding/swc-1259--divider-barebones
branch
from
5966e68 to
e3af985
Compare
There was a problem hiding this comment.
Looks great! 😃
Per specific comments, I'd just like to request a few changes:
-
Add the readonly static property
STATIC_COLORSto the base class, and update the stories to access bothSTATIC_COLORSandVALID_SIZESfrom the component's constructor, instead of importing them directly into module scope. -
Collapse
constdefinitions back intoDivider.types.tsfor now. -
Back the newly added validation code for
staticColorout of this PR and add a@todoinstead. -
Change the
API OVERRIDEScomment header toRENDERING & STYLING
| public get staticColor(): DividerStaticColor | undefined { | ||
| return this._staticColor; | ||
| } | ||
|
|
||
| public set staticColor(value: DividerStaticColor | undefined) { | ||
| if ( | ||
| value != null && | ||
| !(DIVIDER_STATIC_COLORS as readonly string[]).includes(value) | ||
| ) { | ||
| // Silently ignore invalid values | ||
| this._staticColor = undefined; | ||
| return; | ||
| } | ||
| this._staticColor = value; | ||
| } | ||
|
|
||
| private _staticColor?: DividerStaticColor; |
There was a problem hiding this comment.
A couple of things:
-
As a matter of course, I'd prefer that we not include functional changes as part of the migration PRs; I'd rather that, when we come across a potential fix or improvement like this during migration, we add a
@todoand come back to it in a separate PR. -
When we do need to do runtime validation against structured Spectrum type data, I'd like to consistently access that data off of the component's constructor. Accessing it directly from module scope like this means that, if things were to change down the road and this particular data type started varying from one concrete implementation to another, this logic would not be accessing the correct data.
| // ──────────────────── | ||
| // API OVERRIDES | ||
| // ──────────────────── | ||
|
|
There was a problem hiding this comment.
I think this should actually be RENDERING & STYLING, not API OVERRIDES.
| import { | ||
| DIVIDER_STATIC_COLORS, | ||
| DIVIDER_VALID_SIZES, | ||
| } from '@swc/core/components/divider'; |
There was a problem hiding this comment.
These are the imports we can avoid if we systematically hang the structured Spectrum data type arrays off of the component's constructor.
Side note: You can actually already avoid the import of DIVIDER_VALID_SIZES, because SizedMixin itself already hangs VALID_SIZES off of the constructor.
rise-erpelding
requested review from
Rajdeepc,
castastrophe,
graynorton,
marissahuysentruyt and
rubencarvalho
| validSizes: DIVIDER_VALID_SIZES, | ||
| noDefaultSize: true, | ||
| }) { | ||
| static readonly STATIC_COLORS = DIVIDER_STATIC_COLORS; |
There was a problem hiding this comment.
Sorry, missed this earlier, but we should include a doc block and mark it as @internal, and explicitly type it to be readonly string[], as in this example from Badge:
/**
* @internal
*/
static readonly FIXED_VALUES: readonly string[] = FIXED_VALUES;
In cases like this, where it isn't being overridden, the explicit typing isn't strictly necessary, but I'd like to do it as a matter of consistency, as TypeScript does complain if we don't explicitly type and then override in a subclass.
Since it's late in the afternoon and you're on PTO starting tomorrow, I'm going to go ahead and approve the PR rather than request another change; we can always add a commit or edit after merging.
marissahuysentruyt
force-pushed
the
rise-erpelding/swc-1259--divider-barebones
branch
from
c24c5b7 to
ae7081a
Compare
| } | ||
|
|
||
| /* windows high contrast mode */ | ||
| @media (forced-colors: active) { |
marissahuysentruyt
deleted the
rise-erpelding/swc-1259--divider-barebones
branch
6 participants
Description
This PR migrates the next-gen Divider component to Spectrum 2 styles and updates Storybook accordingly.
It also introduces a new Storybook decorator for handling static color backgrounds.
Motivation and context
This PR migrates the Divider component from first-gen to the second-gen architecture, following the established pattern of separating core component logic from Spectrum-specific implementation.
Changes:
What changed
First-gen:
first-gen/packages/divider/src/Divider.ts- Remains unchanged for backward compatibilityCore (Base):
second-gen/packages/core/components/divider/Divider.base.ts- Updates shared functionalitiessecond-gen/packages/core/components/divider/Divider.types.ts- Creates types file for size and static color variantssecond-gen/packages/core/components/divider/index.ts- Imports and exports typesSecond-gen:
second-gen/packages/swc/components/divider/Divider.ts- Updates render method to accommodate classessecond-gen/packages/swc/components/divider/divider.css- Adds CSS fromspectrum-csssecond-gen/packages/swc/components/divider/stories/divider.stories.ts- Adds comprehensive story coveragesecond-gen/packages/swc/.storybook/decorators/static-color-background.ts- Creates decorator for static color backgroundssecond-gen/packages/swc/.storybook/preview.ts- Registers the static color decoratorRelated issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeatures (Note: I did not add a changeset, I wasn't sure that this was set up for barebones and didn't see any for progress circle or badge, but I'm happy to add one if I'm mistaken!)Manual review test cases
Run second-gen Storybook:
Storybook functionality:
s,m,l) to change sizeDocumentation:
Static color decorator:
static-color(as seen in Divider and Progress Circle)First-gen verification (use the PR preview or run locally):
yarn start # or yarn docs:startCode review checklist:
Device review