-
Notifications
You must be signed in to change notification settings - Fork 233
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.
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?
Conversation
|
📚 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... |
b2f7410
to
76955a7
Compare
second-gen/packages/core/components/progress-circle/ProgressCircle.base.ts
Outdated
Show resolved
Hide resolved
second-gen/packages/core/components/progress-circle/ProgressCircle.base.ts
Outdated
Show resolved
Hide resolved
typeof this.fixed !== 'undefined', | ||
})} | ||
> | ||
${when( |
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 updated the ternary to use when
instead but let me know if there's a reason not to use that here.
</div> | ||
` | ||
)} | ||
<div class="spectrum-Badge-label"> |
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 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.
@property({ type: Boolean, reflect: true }) | ||
public subtle: boolean = false; | ||
|
||
public set fixed(fixed: FixedValues | undefined) { |
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 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.
second-gen/packages/swc/components/progress-circle/stories/progress-circle.stories.ts
Outdated
Show resolved
Hide resolved
second-gen/packages/swc/components/progress-circle/stories/progress-circle.stories.ts
Show resolved
Hide resolved
second-gen/packages/swc/components/progress-circle/ProgressCircle.ts
Outdated
Show resolved
Hide resolved
import { ObserveSlotText } from '@swc/core/shared/observe-slot-text'; | ||
|
||
export const BADGE_VARIANTS = [ | ||
export const BADGE_VARIANTS_SEMANTIC = [ |
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.
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.
BadgeBase, | ||
} from '@swc/core/components/badge'; | ||
|
||
export const BADGE_VARIANTS_COLOR = [ |
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 re-exports the const with the addition of the new colors.
...BADGE_VARIANTS_SEMANTIC, | ||
...BADGE_VARIANTS_COLOR, | ||
] as const; | ||
export type BadgeVariant = (typeof BADGE_VARIANTS)[number]; |
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'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.
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.
@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?
|
||
/** | ||
* @element sp-badge-base | ||
* @slot - The text label to display in the badge. |
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.
Since this doesn't have a render, should we define these only on the render layer?
e00d4cc
to
3924d40
Compare
second-gen/packages/core/components/progress-circle/ProgressCircle.base.ts
Outdated
Show resolved
Hide resolved
second-gen/packages/core/components/progress-circle/ProgressCircle.base.ts
Outdated
Show resolved
Hide resolved
second-gen/packages/core/components/progress-circle/ProgressCircle.base.ts
Show resolved
Hide resolved
@castastrophe The S2 addition of these are sooooo lovely!! I'd love to contribute to this!! |
3924d40
to
6bebb21
Compare
* @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 |
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.
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)?
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.
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?
gap: 'var(--spectrum-spacing-200)', | ||
'flex-wrap': 'wrap', | ||
'justify-content': 'center', | ||
// Used 80ch because that's generally considered the maximum readable width for text in a web page. |
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.
👏 ❤️
6bebb21
to
87fe0e1
Compare
87fe0e1
to
e670fbe
Compare
import { ObserveSlotText } from '@swc/core/shared/observe-slot-text'; | ||
|
||
export const BADGE_VARIANTS = [ | ||
export const BADGE_VARIANTS_SEMANTIC = [ |
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.
@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?
15bc02b
to
8e6f89b
Compare
Description
Using the approach outlined in the styling RFC; brought in the existing S2 stylings from the CSS
spectrum-two
branch and noted any questions, concerns, and API implications as comments on this PR.Focused on the
badge
component.Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review