-
Notifications
You must be signed in to change notification settings - Fork 234
feat(status-light)!: migrate status-light to s2 styles and second-gen architecture #5800
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
base: barebones
Are you sure you want to change the base?
feat(status-light)!: migrate status-light to s2 styles and second-gen architecture #5800
Conversation
|
778adb4 to
faba5c8
Compare
- imports new types - adds the api overrides for color and semantic variants - adds documentation - ensures the disabled state is not supported in S2 and throws a warning - ensures the disabled property still adds the aria-disabled attribute for S1 - ensures the variants values are validated and throws a warning if not
faba5c8 to
f32d35c
Compare
📚 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... |
- imports new S2 types for colors and semantic variants - adds the api overrides - adds documentation and example usage - refactors template render to use CSS classes and classMap()
- imports new S1 types for colors and semantic variants - adds the api overrides and extra disabled property - adds documentation, deprecation notices, and example usage
f32d35c to
a22afc0
Compare
a22afc0 to
888651e
Compare
| // ─────────────────────── | ||
|
|
||
| /** | ||
| * @todo This property is deprecated in S2, and can be removed once we are no longer maintaining S1. |
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.
Whoops- should this be @deprecated instead?
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.
Yes, I think we should just go ahead and deprecate this, with an appropriate customer-facing message, similar to the ones for the other deprecations in this file.
| } | ||
|
|
||
| .spectrum-StatusLight--accent:before { | ||
| :host([variant="accent"]):before { |
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.
The 2 updates in the first-gen CSS correspond to the gaps that were identified in the component docs. Is it still in scope for me to correct these missing variants? It obviously introduces VRT diffs, so I'm not sure where or if or when we want to introduce those diffs.
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.
Interesting! Without digging too deeply, it looks to me like these were just cases where our (now-retired) automatic conversion of Spectrum CSS styles to SWC styles was failing—i.e., these were just bugs. Does that seem right to you?
As a matter of principle, my preference would be to split these fixes out into a separate PR, with a changeset describing the fixes.
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 think it makes sense personally but maybe we can also open PRs to fix it on main?
| }; | ||
|
|
||
| export const s = (): TemplateResult => html` | ||
| <sp-status-light size="s" variant="accent">accent</sp-status-light> |
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.
Same question here- we found SWC was missing the accent and cyan variants that are defined for S1. Is it within the scope of this ticket to show those now for the first-gen stories?
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.
Yeah—per my earlier comment, I'd prefer that this and the corresponding fixes be pulled out into a separate PR.
| expect(el.getAttribute('aria-disabled')).to.equal('true'); | ||
| }); | ||
|
|
||
| describe('dev mode warnings', () => { |
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 had Cursor help me with this- do we want tests for the dev warnings at all? Feedback no this is appreciated either way!
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 seems like a good idea to me, but open to dissenting opinions (cc @rubencarvalho).
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 think we do test for dev mode on other components so I think that makes a lot of sense to add here too. Check out this reference for an example:
spectrum-web-components/first-gen/packages/alert-banner/test/alert-banner.test.ts
Line 193 in ee5fdd5
| it('should log dev warning when given invalid variant', async () => { |
| argTypes.variant = { | ||
| ...argTypes.variant, | ||
| name: 'Variant', | ||
| description: | ||
| 'Changes the color of the status dot. The variant list includes both semantic and non-semantic options.', | ||
| type: { name: 'string', required: true }, | ||
| table: { | ||
| type: { summary: 'string' }, | ||
| defaultValue: { summary: 'info' }, | ||
| category: 'Component', | ||
| }, | ||
| control: { type: 'select' }, | ||
| options: StatusLight.VARIANTS, | ||
| }; |
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.
Feel free to tell me to remove this and just follow what Badge is doing! This was the way I found that worked to populate more of the control table, but maybe that's not something we want to dive into right now. (and also this approach didn't really work with the default slot arg, sooo...it's not fool-proof 😆)
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.
Love 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.
Our vision for where this is going is that pretty much everything populating the table will flow from our code (including JSDocs) all the way through to Storybook, with little to no need for manual intervention in the *.stories.ts files.
To the extent that's not happening yet, I'd like to see if we can diagnose and fix issues in our tooling pipeline.
Meanwhile, I'd like to see us take a light touch, making minor tweaks as needed to unblock / ease testing during development, but otherwise not striving to manually fill gaps between what's currently showing and where we'd eventually like to get.
Since we're not expecting customers to be referring to the 2nd-gen Storybook for some time yet, the rough edges shouldn't hurt anyone, and their continued presence should help incentivize us to address the root-cause issues in the tool chain! 😁
If I look at what you've done here and compare it to the equivalent of what's being done in Badge:
argTypes.variant = {
...argTypes.variant,
control: { type: 'select' },
options: StatusLight.VARIANTS,
};
...they seem essentially equivalent in terms of behavior, and your improvement to the description can be achieved by editing the JSDoc in component implementation, so I'd like to see us fall back to the lighter version. Does that make sense? Let me know if I'm missing anything significant.
(Ultimately, I'm hoping we don't even have to do this much augmentation—I think we should be able to dial in our tooling to the point where it all "just works.")
| }; | ||
|
|
||
| // ──────────────────────── | ||
| // HELPER FUNCTIONS |
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.
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.
Cursor pulled in the badge test patterns and created this status light test file. Is this something appropriate for the migration, or would you like me to separate this out into a different PR?
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.
Technically speaking, tests weren't required for these Barebones component migrations.
I am neutral on whether we take these as part of this PR or pull them out and consider testing more holistically across the Barebones components.
@rubencarvalho, thoughts?
| * We may have to explicitly bind the args to the component (particularly | ||
| * helpful for the size property) so the Storybook controls work as expected. | ||
| * | ||
| * i.e. render: (args) => | ||
| html`<swc-status-light .size=${args.size} variant=${args.variant} | ||
| >${args['default-slot']}</swc-status-light | ||
| >`, |
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.
If this comment is too much/overkill, let me know. I did document this idea in #5798, as well.
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.
The getStorybookHelpers function is supposed to automatically handle the bindings, but it's not working as expected for certain properties; in the case of size, I think it's probably because properties from mixins are not propagating as they should through the Custom Element Manifest generation process.
I see this falling into the same category I was discussing in my previous comment—our preferred option is to see if we can make our tooling "just work", so we don't have to manually bind templates in our stories. If we try that and fail, then I think we can fall back to something like this.
| args: { | ||
| ['default-slot']: 'Status light', | ||
| variant: 'info', | ||
| size: 'm', |
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.
Does this conflict with the noDefaultSize arg in StatusLightBase (for SizedMixin)? I don't have a good handle on the SizedMixin, but there is a default size defined in the design specs.
Maybe I need to do a Cursor dive into SizedMixin!
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 is a good question! SizedMixin is not too complex, so the implications of noDefaultSize should be easy enough to reason about. Not having thought through the nuances, and without knowing the history, it's hard to say why SWC would be applying noDefaultSize if the design does, in fact, call for a default size, but I'm hopeful we'll be able to make sense of it.
Meanwhile, I would guess that there's no issue with having a default size from a Storybook perspective, even if there are legitimate technical reasons why noDefaultSize is the right choice from a SizedMixin point of view.
That said, for the sake of consistency in the way we're overriding args in our Barebones stories, I'd prefer that we insert these lines above the declaration of meta:
args['default-slot'] = 'Status light';
args.size = 'm';
...and then just refer to args in the meta declaration, rather than define it inline, e.g.:
const meta: Meta = {
/* other props... */
args,
};
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 looks really great to me. Thanks for tackling this!
| } | ||
|
|
||
| .spectrum-StatusLight--accent:before { | ||
| :host([variant="accent"]):before { |
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 think it makes sense personally but maybe we can also open PRs to fix it on main?
| expect(el.getAttribute('aria-disabled')).to.equal('true'); | ||
| }); | ||
|
|
||
| describe('dev mode warnings', () => { |
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 think we do test for dev mode on other components so I think that makes a lot of sense to add here too. Check out this reference for an example:
spectrum-web-components/first-gen/packages/alert-banner/test/alert-banner.test.ts
Line 193 in ee5fdd5
| it('should log dev warning when given invalid variant', async () => { |
| argTypes.variant = { | ||
| ...argTypes.variant, | ||
| name: 'Variant', | ||
| description: | ||
| 'Changes the color of the status dot. The variant list includes both semantic and non-semantic options.', | ||
| type: { name: 'string', required: true }, | ||
| table: { | ||
| type: { summary: 'string' }, | ||
| defaultValue: { summary: 'info' }, | ||
| category: 'Component', | ||
| }, | ||
| control: { type: 'select' }, | ||
| options: StatusLight.VARIANTS, | ||
| }; |
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.
Love 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.
This looks really good—nice work! 😊
Most of my comments are about places where you've gone above and beyond Barebones requirements in ways that I think we may either want to pull out into separate PRs or just hold off on for right now—let's discuss in the comment threads.
| } | ||
|
|
||
| .spectrum-StatusLight--accent:before { | ||
| :host([variant="accent"]):before { |
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.
Interesting! Without digging too deeply, it looks to me like these were just cases where our (now-retired) automatic conversion of Spectrum CSS styles to SWC styles was failing—i.e., these were just bugs. Does that seem right to you?
As a matter of principle, my preference would be to split these fixes out into a separate PR, with a changeset describing the fixes.
| // ─────────────────────── | ||
|
|
||
| /** | ||
| * @todo This property is deprecated in S2, and can be removed once we are no longer maintaining S1. |
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.
Yes, I think we should just go ahead and deprecate this, with an appropriate customer-facing message, similar to the ones for the other deprecations in this file.
| }; | ||
|
|
||
| export const s = (): TemplateResult => html` | ||
| <sp-status-light size="s" variant="accent">accent</sp-status-light> |
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.
Yeah—per my earlier comment, I'd prefer that this and the corresponding fixes be pulled out into a separate PR.
| expect(el.getAttribute('aria-disabled')).to.equal('true'); | ||
| }); | ||
|
|
||
| describe('dev mode warnings', () => { |
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 seems like a good idea to me, but open to dissenting opinions (cc @rubencarvalho).
| // Check disabled property if it exists (S1 only) | ||
| if (this.hasAttribute('disabled') && !('disabled' in this)) { | ||
| window.__swc.warn( | ||
| this, | ||
| `<${this.localName}> element does not support the disabled state.`, | ||
| 'https://opensource.adobe.com/spectrum-web-components/components/status-light/#states', | ||
| { | ||
| issues: [ | ||
| 'disabled is not a supported property in Spectrum 2', | ||
| ], | ||
| } | ||
| ); | ||
| } else if (this.hasAttribute('disabled') && 'disabled' in this) { | ||
| // Set aria-disabled when no warning is shown | ||
| if (!this.hasAttribute('aria-disabled')) { | ||
| this.setAttribute('aria-disabled', 'true'); | ||
| } else { | ||
| this.removeAttribute('aria-disabled'); | ||
| } | ||
| } |
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 case is interestingly different from the similar case in Badge, and I think we should do something slightly different here.
In Badge, we're doing validation of a feature introduced in S2 (outline), which is expected to carry on into the foreseeable future, so it seemed to make sense to put that validation logic in the base class rather than in the 2nd-gen SWC subclass.
In this case, we have two bits of logic related to a feature being removed in S2 (disabled):
- Validation to warn when customers try to apply
disabledin an S2 context - S1-specific logic for managing state related to the
disabledfeature
I'd argue that the latter should just live in the S1-specific subclass, where it will eventually fade away into oblivion without further action from us.
The former can stay here, at least for now. @rubencarvalho and I have been discussing the fact that, before going much further, we should decide on an overall philosophy regarding runtime validation of 1st-gen-to-2nd-gen changes (how much of it we want to do, and in what cases).
| argTypes.variant = { | ||
| ...argTypes.variant, | ||
| name: 'Variant', | ||
| description: | ||
| 'Changes the color of the status dot. The variant list includes both semantic and non-semantic options.', | ||
| type: { name: 'string', required: true }, | ||
| table: { | ||
| type: { summary: 'string' }, | ||
| defaultValue: { summary: 'info' }, | ||
| category: 'Component', | ||
| }, | ||
| control: { type: 'select' }, | ||
| options: StatusLight.VARIANTS, | ||
| }; |
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.
Our vision for where this is going is that pretty much everything populating the table will flow from our code (including JSDocs) all the way through to Storybook, with little to no need for manual intervention in the *.stories.ts files.
To the extent that's not happening yet, I'd like to see if we can diagnose and fix issues in our tooling pipeline.
Meanwhile, I'd like to see us take a light touch, making minor tweaks as needed to unblock / ease testing during development, but otherwise not striving to manually fill gaps between what's currently showing and where we'd eventually like to get.
Since we're not expecting customers to be referring to the 2nd-gen Storybook for some time yet, the rough edges shouldn't hurt anyone, and their continued presence should help incentivize us to address the root-cause issues in the tool chain! 😁
If I look at what you've done here and compare it to the equivalent of what's being done in Badge:
argTypes.variant = {
...argTypes.variant,
control: { type: 'select' },
options: StatusLight.VARIANTS,
};
...they seem essentially equivalent in terms of behavior, and your improvement to the description can be achieved by editing the JSDoc in component implementation, so I'd like to see us fall back to the lighter version. Does that make sense? Let me know if I'm missing anything significant.
(Ultimately, I'm hoping we don't even have to do this much augmentation—I think we should be able to dial in our tooling to the point where it all "just works.")
| * We may have to explicitly bind the args to the component (particularly | ||
| * helpful for the size property) so the Storybook controls work as expected. | ||
| * | ||
| * i.e. render: (args) => | ||
| html`<swc-status-light .size=${args.size} variant=${args.variant} | ||
| >${args['default-slot']}</swc-status-light | ||
| >`, |
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.
The getStorybookHelpers function is supposed to automatically handle the bindings, but it's not working as expected for certain properties; in the case of size, I think it's probably because properties from mixins are not propagating as they should through the Custom Element Manifest generation process.
I see this falling into the same category I was discussing in my previous comment—our preferred option is to see if we can make our tooling "just work", so we don't have to manually bind templates in our stories. If we try that and fail, then I think we can fall back to something like this.
| args: { | ||
| ['default-slot']: 'Status light', | ||
| variant: 'info', | ||
| size: 'm', |
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 is a good question! SizedMixin is not too complex, so the implications of noDefaultSize should be easy enough to reason about. Not having thought through the nuances, and without knowing the history, it's hard to say why SWC would be applying noDefaultSize if the design does, in fact, call for a default size, but I'm hopeful we'll be able to make sense of it.
Meanwhile, I would guess that there's no issue with having a default size from a Storybook perspective, even if there are legitimate technical reasons why noDefaultSize is the right choice from a SizedMixin point of view.
That said, for the sake of consistency in the way we're overriding args in our Barebones stories, I'd prefer that we insert these lines above the declaration of meta:
args['default-slot'] = 'Status light';
args.size = 'm';
...and then just refer to args in the meta declaration, rather than define it inline, e.g.:
const meta: Meta = {
/* other props... */
args,
};
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.
Technically speaking, tests weren't required for these Barebones component migrations.
I am neutral on whether we take these as part of this PR or pull them out and consider testing more holistically across the Barebones components.
@rubencarvalho, thoughts?

Description
This PR migrates the status-light component to the second-generation architecture, creating a shared base class that both first-gen and second-gen implementations extend. This enables the component to work in both Spectrum 1 and Spectrum 2 contexts while sharing common logic.
Key changes
@swc/core/components/status-light/StatusLight.base.ts) that contains common logic for both generationsStatusLight.types.ts) defining variants for S1 and S2:first-gen/packages/status-light/src/StatusLight.ts) to extend the shared base classdisabledproperty (deprecated in S2)second-gen/packages/swc/components/status-light/StatusLight.ts) extending the shared base classdisabledproperty (not supported in Spectrum 2)Motivation and context
This migration is part of the broader effort to support Spectrum 2 design system while maintaining backward compatibility with Spectrum 1. By creating a shared base class, we reduce code duplication and ensure consistent behavior across both generations while allowing each to have generation-specific features and styling.
Related issue(s)
Screenshots (if appropriate)
New stories for S2/second-gen:



Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
First-gen status-light functionality
NOTE: these missing variants were an identified gap during the component analysis
disabledstate still works correctly and properly adds or removesaria-disabledSecond-gen status-light functionality
Shared base class validation
<sp-status-light size="s" variant="brown">brown</sp-status-light>Second-gen: Try adding an extra S1-only story to the S2 storybook. For example, add:
Confirm that a browser warning appears in the console:
Replace the variant value with something valid for S2, then reconfirm the new
disabledconsole warning appears in the browser:Device review