-
Notifications
You must be signed in to change notification settings - Fork 233
feat(progress-circle): migrate progress circle component to second-generation architecture #5743
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(progress-circle): migrate progress circle component to second-generation architecture #5743
Conversation
|
e670fbe
to
5b68b5d
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 |
5b68b5d
to
f9dba02
Compare
Tachometer resultsCurrently, no packages are changed by this PR... |
f9dba02
to
059fbd3
Compare
059fbd3
to
1794e2c
Compare
1794e2c
to
1231083
Compare
const radius = `calc(50% - ${strokeWidth / 2}px)`; | ||
|
||
return html` | ||
<slot @slotchange=${this.handleSlotchange}></slot> |
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.
For the sake of simplifying this for S2, I've regressed some of this functionality by removing the slot and leveraging our SVG approach. Would love to have a larger conversation around this for S2 but this seemed sufficient for the base requirement of getting the S2 styles and rendering into the component.
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, I don't really see a good reason for us to continue supporting both the slot-based and attribute-based interfaces for providing accessible label text.
That said, I would like to decouple that decision from the initial Barebones component migration, to keep Barebones scoped as minimally as possible.
On the branch where I've been exploring the nuances of our layering / inheritance scheme, I experimentally re-added the slot to the new, SVG-based render()
implementation, and it seemed to work fine; just needed to set display: none;
on the slot to echo what's done in 1st-gen.
No need to reverse the change on this branch, but I will include the slot re-addition in my follow-up PR, along with a @todo
to make a decision on deprecation.
render: () => html` | ||
<div | ||
style="background-color: rgba(0,0,0,0.4); padding: 24px; display: flex; gap: 24px; align-items: center;" | ||
style="background: linear-gradient(45deg, rgb(64 0 22), rgb(14 24 67)); padding: 24px; display: flex; gap: 24px; align-items: center;" |
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.
Added our pretty gradients here for static white/black which match how design demos these styles. They're meant to be on visually busy backgrounds like gradients or images so that drives home that guidance in Storybook as well.
`, | ||
}; | ||
|
||
export const WithSlottedContent: Story = { |
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 removed the slot for the purposes of my S2 update but we can always bring this back.
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.
Per my earlier comment, I don't want to remove the slot just yet, but I'm fine with removing the story.
Per the minimalistic Barebones requirements, the state of each component's stories is not a thing we're worrying about for Barebones. We'll circle back after the Barebones milestone to do a holistic look at the stories for all 5 initial components and try to establish some best practices and patterns at that point.
|
||
import progressCircleStyles from './progress-circle.css'; | ||
|
||
function capitalize(str?: string): string { |
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 kind of liked this utility living separately but let me know how you want to handle this in the SWC infrastructure.
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.
For now, what you've done here looks great. Again, we can circle back after Barebones to discuss how we want to handle utilities like this more generally in 2nd-gen.
second-gen/packages/swc/components/progress-circle/ProgressCircle.ts
Outdated
Show resolved
Hide resolved
1231083
to
6360869
Compare
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've left a bunch of individual comments that I somehow managed not to include as part of this review 😛, but this looks good to me, and I'd like to proceed with merging into Barebones.
As noted in my individual comments, we can continue to iterate in place.
Description
Updates the 2nd-gen Progress Circle component to the latest CSS styles from Spectrum CSS
spectrum-two
branch. This updates Progress Circle for the barebones milestone components with more complete S2 functionality including determinate/indeterminate states, sizing variants, and static color options.Note: there are known rendering issues with the SVG being cut off at the bottom but it has been approved to move forward and resolve that issue at a later time.
Related issue(s)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Verify Progress Circle component functionality
Validate indeterminate state
Test sizing variants
Verify static color variants
Validate accessibility features