Skip to content

feat(badge): s2 styling and render #5718

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 2, 2025
Merged

feat(badge): s2 styling and render #5718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe12a3c
feat(badge): s2 styling and render brought in
castastrophe Sep 3, 2025
e89e302
fix(badge,progress-circle): remove modifiers
castastrophe Sep 3, 2025
e1d2421
fix(badge): separate S1/S2 API in alternate approach
castastrophe Sep 3, 2025
b52e52a
chore: adding docs notes and todos to 2nd gen
castastrophe Sep 3, 2025
5b47c1c
chore: remove progress-circle into separate PR
castastrophe Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 17 additions & 23 deletions second-gen/packages/core/components/badge/Badge.base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@ import { SizedMixin, SpectrumElement } from '@swc/core/shared/base';
import { ObserveSlotPresence } from '@swc/core/shared/observe-slot-presence';
import { ObserveSlotText } from '@swc/core/shared/observe-slot-text';

export const BADGE_VARIANTS = [
export const BADGE_VARIANTS_SEMANTIC = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separating out the semantic variants from the color variants helps us in Storybook (and possibly down the road) because only semantic variants support outline styling.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this seems like a good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubencarvalho Initially I started updating these assets per the S2 guidelines before I realized this is the base class being imported into S1 components as well. Would it make sense to hoist this folder to sit between the first- and second-get folders so it's clear that it's a shared resource?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key here is just going to be to make it clear (via docs and other means) that the S1/S2 distinction is not the same as the 1st-gen/2nd-gen distinction.

The core library is in 2nd-gen because it, along with 2nd-gen SWC, is going to use our 2nd-gen tooling and infrastructure, and because we have a sense that the core layer may prove useful even after the 1st-gen to 2nd-gen transition is complete. (If it does not, we can remove it when we eventually stop supporting 1st-gen.)

As a layer of the architecture that explicitly supports both S1 and S2, I think it's actually OK if core ends up including some type definitions that are S1- and S2-aware, allowing us to manage the differences in one place. I've done some further exploration of what this might look like that I'll put in a separate PR.

'accent',
'neutral',
'informative',
'positive',
'negative',
'notice',
] as const;

export const BADGE_VARIANTS_COLOR = [
'fuchsia',
'indigo',
'magenta',
Expand All @@ -38,6 +41,11 @@ export const BADGE_VARIANTS = [
'cyan',
'blue',
] as const;

export const BADGE_VARIANTS = [
...BADGE_VARIANTS_SEMANTIC,
...BADGE_VARIANTS_COLOR,
] as const;
export type BadgeVariant = (typeof BADGE_VARIANTS)[number];
export const FIXED_VALUES = [
'inline-start',
Expand All @@ -49,8 +57,9 @@ export type FixedValues = (typeof FIXED_VALUES)[number];

/**
* @element sp-badge-base
* @slot - The text label to display in the badge.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this doesn't have a render, should we define these only on the render layer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to define them here on the base class, since these slots are effectively part of the component architecture dictated by the base class (i.e., any concrete implementation building on this base class will need to include the slots), and the base class includes the mixin functionality associated with the slots.

* @slot icon - The icon to display in the badge.
* @attribute {ElementSize} size - The size of the badge.
* @attribute {BadgeVariant} variant - The variant of the badge.
* @attribute {FixedValues} fixed - The fixed position of the badge.
*/
export abstract class BadgeBase extends SizedMixin(
ObserveSlotText(ObserveSlotPresence(SpectrumElement, '[slot="icon"]'), ''),
Expand All @@ -61,27 +70,12 @@ export abstract class BadgeBase extends SizedMixin(
@property({ type: String, reflect: true })
public variant: BadgeVariant = 'informative';

@property({ reflect: true })
public get fixed(): FixedValues | undefined {
return this._fixed;
}

public set fixed(fixed: FixedValues | undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to me to be how the @Property works by default. I don't see any regressions when I simplify this but let me know if I've missed a nuance here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a glance, I think you're right—at least, I can't immediately see why this property would need a bespoke getter and setter.

That said, the aim of the Barebones milestone is to avoid any changes not strictly related to migrating these components to the new architecture, so I think the best course of action here would be to add a @todo comment rather than making a change in this PR.

if (fixed === this.fixed) {
return;
}
const oldValue = this.fixed;
this._fixed = fixed;
if (fixed) {
this.setAttribute('fixed', fixed);
} else {
this.removeAttribute('fixed');
}
this.requestUpdate('fixed', oldValue);
}

private _fixed?: FixedValues;
@property({ type: String, reflect: true })
public fixed?: FixedValues;

/**
* @internal Used for rendering gap when the badge has an icon.
*/
protected get hasIcon(): boolean {
return this.slotContentIsPresent;
}
Expand Down
108 changes: 90 additions & 18 deletions second-gen/packages/swc/components/badge/Badge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,40 @@
* governing permissions and limitations under the License.
*/

import { CSSResultArray, html, nothing, TemplateResult } from 'lit';
import { CSSResultArray, html, PropertyValues, TemplateResult } from 'lit';
import { property } from 'lit/decorators.js';
import { classMap } from 'lit/directives/class-map.js';
import { when } from 'lit/directives/when.js';

import { BadgeBase } from '@swc/core/components/badge';
import {
BADGE_VARIANTS_COLOR as BADGE_VARIANTS_COLOR_BASE,
BADGE_VARIANTS_SEMANTIC,
BadgeBase,
} from '@swc/core/components/badge';

export const BADGE_VARIANTS_COLOR = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This re-exports the const with the addition of the new colors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we definitely need to define a constant representing the full set of S2 colors. As I mentioned in an earlier comment, I've done some additional exploration into patterns for managing differences in S1 & S2 types, which I'll put in a separate PR (to land after this one).

...BADGE_VARIANTS_COLOR_BASE,
'pink',
'turquoise',
'brown',
'cinnamon',
'silver',
] as const;

export const BADGE_VARIANTS = [
...BADGE_VARIANTS_SEMANTIC,
...BADGE_VARIANTS_COLOR,
] as const;
export type BadgeVariant = (typeof BADGE_VARIANTS)[number];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I need to redefine this since technically it's the same but the content of the BADGE_VARIANTS has changed so I went ahead and did that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubencarvalho Do you know how that works in Typescript? Would it calculate the value of BadgeVariant at the time of use based on the new definition of BADGE_VARIANTS?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as far as TypeScript is concerned, the type defined in this module is completely different from the type defined in the core module, though they share the same name, so it would in fact need to be exported here.

Again, though, I'll make a separate follow-on PR to propose some patterns for managing S1 and S2 types.


import styles from './badge.css';

// Export types and values to avoid breaking changes
export { BADGE_VARIANTS, FIXED_VALUES } from '@swc/core/components/badge';
export type { BadgeVariant, FixedValues } from '@swc/core/components/badge';
export {
BADGE_VARIANTS_SEMANTIC,
FIXED_VALUES,
} from '@swc/core/components/badge';
export type { FixedValues } from '@swc/core/components/badge';

/**
* A badge component that displays short, descriptive information about an element.
Expand All @@ -28,14 +53,16 @@ export type { BadgeVariant, FixedValues } from '@swc/core/components/badge';
* @since 1.0.0
* @status stable
* @github https://github.com/adobe/spectrum-web-components/tree/main/...
* @figma https://www.figma.com/design/...
* @figma https://www.figma.com/design/Mngz9H7WZLbrCvGQf3GnsY/S2-%2F-Desktop?node-id=36806-6551
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At one point, we talked about not sharing Figma links. Are we concerned at all with committing this link to the repo? I know we have it in CSS, but I thought that ended up being sort of a no-no. Or was that just for in PR descriptions? Or am I misremembering (which could totally be)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! I'm not sure. I know we don't post Jira links publicly but I thought since the Figma assets are behind a login, it was okay to include them in Storybook. @najikahalsema do you know?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, since we're not going to be releasing any S2 components for a while (even in preview form), we might as well take the conservative approach and not include any real Figma links for the time being.

*
* @attribute {BadgeVariant} variant - The variant of the badge.
* @attribute {boolean} subtle - Whether the badge is subtle.
* @attribute {boolean} outline - Whether the badge is outlined.
* @attribute {FixedValues} fixed - The fixed position of the badge.
*
* @slot - Text label of the badge
* @slot icon - Optional icon that appears to the left of the label
*
* @csspart label - The text content area of the badge
* @csspart icon - The icon area of the badge (when present)
*
* @example
* <swc-badge variant="positive">New</swc-badge>
*
Expand All @@ -46,23 +73,68 @@ export type { BadgeVariant, FixedValues } from '@swc/core/components/badge';
* </swc-badge>
*/
export class Badge extends BadgeBase {
@property({ type: Boolean, reflect: true })
public subtle: boolean = false;

@property({ type: Boolean, reflect: true })
public outline: boolean = false;

public static override get styles(): CSSResultArray {
return [styles];
}

protected override render(): TemplateResult {
return html`
${this.hasIcon
? html`
<slot
name="icon"
?icon-only=${!this.slotHasContent}
></slot>
`
: nothing}
<div class="label">
<slot></slot>
<div
class=${classMap({
['spectrum-Badge']: true,
[`spectrum-Badge--size${this.size?.toUpperCase()}`]:
typeof this.size !== 'undefined',
[`spectrum-Badge--${this.variant}`]:
typeof this.variant !== 'undefined',
[`spectrum-Badge--subtle`]: this.subtle,
[`spectrum-Badge--outline`]: this.outline,
[`spectrum-Badge--fixed-${this.fixed}`]:
typeof this.fixed !== 'undefined',
})}
>
${when(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the ternary to use when instead but let me know if there's a reason not to use that here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Since the 2nd-gen render() method is conceptually free to diverge from the 1st-gen method in whatever way is useful (provided there is no impact on the component API), there's no reason not to opt for when here.

this.hasIcon,
() => html`
<div
class=${classMap({
[`spectrum-Badge-icon`]: true,
[`spectrum-Badge-icon--no-label`]:
!this.slotHasContent,
})}
>
<slot name="icon"></slot>
</div>
`
)}
<div class="spectrum-Badge-label">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested adding this class to the slot itself and it didn't render padding correctly. Maybe just a case of needing the appropriate display settings but I didn't dig into it too much. Would be good to have a standard "best practice" in place for whether we want to wrap slots to apply styles or style slot containers directly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks like adding a rule to the component styles that sets display: block; on the slot also works.

I think in general it would be better to avoid systematically wrapping slots for styling purposes.

<slot></slot>
</div>
</div>
`;
}

protected override update(changedProperties: PropertyValues): void {
super.update(changedProperties);
if (window.__swc?.DEBUG) {
if (
this.outline &&
!BADGE_VARIANTS_SEMANTIC.includes(this.variant)
) {
window.__swc.warn(
this,
`<${this.localName}> element only supports the outline styling if the variant is a semantic color variant.`,
'https://opensource.adobe.com/spectrum-web-components/components/badge/#variants',
{
issues: [...BADGE_VARIANTS_SEMANTIC],
}
);
}
}
}
}
Loading
Loading