-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(progress-circle): migrating s2 tokens, refactor markup to use SVG element #3380
base: spectrum-two
Are you sure you want to change the base?
feat(progress-circle): migrating s2 tokens, refactor markup to use SVG element #3380
Conversation
🦋 Changeset detectedLatest commit: 8dd9bb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
File metricsSummaryTotal size: 1.73 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsprogresscircle
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3380--spectrum-css.netlify.app |
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.
Woo- love the updated styles. I think I wrapped my head around the animation stuff, but if anything is confusing, let's pair! 🍐
A couple of other thoughts I wasn't sure where to put:
-
Cassondra separated some of nested components in the
s2-foundations-redux
branch. For instance, progress bar and meter were nested together, but now they're totally separate (https://github.com/adobe/spectrum-css/tree/s2-foundations-redux/components/meter). Same thing with form being separated from field label. You may have already know, but is there a way in this branch we can prep for infield progress circle being separate from progress circle? Or maybe we leave our comment/note/Jira ticket for ourselves? Should these 2 components be nested like they are now, or should they be separated into different component directories? -
Let's just fix the typo in the file names. (infield-progresscirlce > progresscircle)
@@ -14,45 +16,53 @@ export const Template = ({ | |||
size = "m", | |||
staticColor, | |||
isIndeterminate = false, | |||
isInField = false, |
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 don't see isInField
in the progress circle stories args. Should it be? If we don't want users to change it, we could just do the table: { disable: true, } }
thing. Or does it need to be in the args for infield-progresscircle
?
components/progresscircle/stories/infield-progresscirlce.stories.js
Outdated
Show resolved
Hide resolved
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.
A lot of my first round of suggestions look to be addressed- thank you! I left some more questions and comments for you.
Like we sort of talked about during standup, it might be best to pull that last commit out for infield progress circle onto its own branch. We definitely don't want to lose any of that work, so I'd probably just make a new infield progress circle branch off of this branch first 🤷♀️ Then for this S2 regular progress circle branch, we could trim down the changes related to "old" progress circles becoming the infield progress circles. I have more comments/questions/suggestions for infield progress circle, but I didn't want to clutter this PR if we move that component. I'm up to help with this if anything I just tried to say is confusing!!!
With that in mind, maybe some of my comments (like for picker and combobox) would probably be moved over to that new branch.
I'm also curious if we should check with design about the progress circle that's within the textfield. It's got an isLoading
state on main
, and it looks like isLoading
is still going to be around for S2 for textfield, so I think we may need to double check that the progress circle is in fact the "infield progress circle" in S2. At least that's my assumption. Here's Rise's blocked s2 textfield PR if you want to see.
Let's pair if you want help on any of this. I know I left a ton of questions again. 😬
.spectrum-ProgressCircle--indeterminate-fill-submask-2 { | ||
animation: 1s infinite linear spectrum-fill-mask-2; | ||
.spectrum-ProgressCircle--indeterminate { | ||
animation: 1s infinite linear spectrum-fill-mask-1; |
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 very familiar with this component (and I'm terrible with animations). Would you explain what each of these animations do for me? With the little bit of playing around I did, I'm assuming one is for the track and one is for the fill. Why do we need both on this class?
Should these lines be combined at all? Because right now, only spectrum-fill-mask-2
is rendering right? Because the spectrum-fill-mask-1
line is being overridden by the cascade?
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.
It does absolutely nothing so I'm removing them both
@@ -84,6 +84,7 @@ export const Picker = ({ | |||
...globals, | |||
size: "s", | |||
isIndeterminate: true, | |||
isInField: true |
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.
Do we need to delete this now? Since the infield progress circle is separated out? We should probably use the new component when picker isLoading
instead of the "old" progress circle too?
${when(isLoading, () =>
InfieldProgressCircle({
...globals,
size,
isIndeterminate: true,
})
)}
@@ -81,6 +81,7 @@ export const Template = ({ | |||
testId: "progress-circle", | |||
staticColor, | |||
isIndeterminate: true, | |||
isInField: true |
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- should this isInField: true
get deleted? Instead of using this isInField
arg, should we be importing the new infieldprogresscircle
component (instead of the regular progress circleto use in the ${when(isPending...
block?
${when(isPending, () => InfieldProgressCircle({
...globals,
size,
testId: "progress-circle",
staticColor,
isIndeterminate: true,
}))}
export const StaticWhite = ProgressCircleGroup.bind({}); | ||
StaticWhite.args = { | ||
staticColor: "white", | ||
}; |
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.
We can probably just keep this story too, instead of deleting it. There's several static color stories in main
, so my gut says to keep it, but what do you think? I go back and forth on what presents the least painful conflicts when we finally get this work into main
.
export const StaticWhiteIndeterminate = Sizing.bind({}); |
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.
No harm in keeping it 😁
`; | ||
|
||
export const Default = ProgressCircleGroup.bind({}); | ||
export const Default = Template.bind({}); |
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.
Oh good idea! Instead of binding to the Template
, can we still import and bind to ProgressCircleGroup
? That should match what the Default story is doing on main
. 👍 🙌
export const Default = ProgressCircleGroup.bind({}); |
@@ -80,7 +80,7 @@ export const Template = ({ | |||
], | |||
customInputClasses: [`${rootClass}-input`], | |||
isLoading, | |||
customProgressCircleClasses: ["spectrum-Combobox-progress-circle"], | |||
customProgressCircleClasses: ["spectrum-Combobox-progress-circle spectrum-ProgressCircle--infield"], |
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.
customProgressCircleClasses: ["spectrum-Combobox-progress-circle spectrum-ProgressCircle--infield"], | |
customProgressCircleClasses: ["spectrum-Combobox-progress-circle spectrum-InfieldProgressCircle"], |
@@ -1,4 +1,3 @@ | |||
import { html } from "lit"; | |||
|
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.
Could we delete this empty line?
@@ -1,4 +1,3 @@ | |||
import { html } from "lit"; | |||
|
|||
import { Template } from "./template"; |
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.
Could we add the .js
extension to this import?
import { Template } from "./template.js";
type: { summary: "string" }, | ||
category: "Advanced", | ||
|
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.
Let's remove this empty line.
cdb180d
to
27d01df
Compare
59d0428
to
e6d16bd
Compare
Expanding the existing themes system to support the new S2 mappings. --- Co-authored-by: castastrophe <[email protected]> Co-authored-by: Patrick Fulton <[email protected]> Co-authored-by: Cory Dransfeldt <[email protected]> Co-authored-by: Aziz Ramos <[email protected]> Co-authored-by: Josh Winn <[email protected]> Co-authored-by: Rise Erpelding <[email protected]> Co-authored-by: Marissa Huysentruyt <[email protected]> Co-authored-by: Rajdeep Chandra <[email protected]> Co-authored-by: TarunAdobe <[email protected]> Co-authored-by: Dragan Eror<[email protected]>
* feat: s2 foundations non-gray-800 colors update * add changeset
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
--- Co-authored-by: TarunAdobe <[email protected]>
--- Co-authored-by: TarunAdobe <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* docs(contextualhelp): remove MDX file - adds some additional documentation regarding stories/variants - migrates documentation in MDX to stories.js instead * docs(fieldgroup): remove MDX file - creates additional templates to showcase variants in a single story on the docs page - additional documentation to give context to stories - corrects sentence-casing in story names - removes duplicate control for label text - migrates documentation in MDX to stories.js instead * chore(fieldgroup,contextualhelp): cleans up test files - fieldgroup: we don't really need to change the label text or help text between test cases, so some of the test-specific changes were removed - contextualhelp: add help icon test case * docs(datepicker): remove MDX file - adds missing date picker docs - migrates documentation in MDX to stories.js instead - creates OpenClosedTemplate to display both states in a single story on the docs page - adds Range variant to the sidebar since that control isn't in the table
* docs(accordion): remove MDX file - adds accordion sizing story - migrates documentation in MDX to stories.js instead * docs(actionbar): remove MDX file - adds actionbar flexible and sticky variants to default/emphasized stories - migrates documentation in MDX to stories.js instead - updates references to mods tables * docs(asset): remove MDX file
…3471) * fix(popover): define --spectrum-popover-border-width in index.css * fix(well): define --spectrum-well-border-color in index.css * fix(tabs): define --spectrum-tabs-font-weight * fix(toast): define default background color and divider color * fix(tag): define undefined custom properties * fix(menu): define menu item background colors * fix(textfield): define undefined custom properties
…x and picker to show examples
13e35a1
to
c97dee1
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.
Your rebase cleaned this up so nice! It's looking good, but I do have some questions for you:
Just wanted to be sure- the corner-radius-full
token for the fill isn't used correct? Because we're using the stroke-linecap
attribute? I saw it in the Figma file, but didn't see it used in a quick search of the CSS file. But I'm pretty sure that's why.
There's the changes in progress-bar.stories.js
and tooltip.stories.js
. I vote we revert those and I'll get a bug branch going to fix those pages.
The test files are here! Would you add the static black test case?
// progresscircle.test.js
...
{
testHeading: "Static white",
staticColor: "white",
},
{
testHeading: "Static black",
staticColor: "black",
},
...
.changeset/itchy-kids-travel.md
Outdated
### Added tokens | ||
|
||
`--spectrum-in-field-progress-circle-edge-to-fill` | ||
`--spectrum-in-field-progress-circle-size-75` | ||
`--spectrum-in-field-progress-circle-size-100` | ||
`--spectrum-in-field-progress-circle-size-200` | ||
`--spectrum-in-field-progress-circle-size-300` |
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.
ooo should these go in the infield progress circle changeset?
.changeset/itchy-kids-travel.md
Outdated
|
||
## Progress circle S2 migration | ||
|
||
This is the S2 migration of the progress circle. It has changed to an svg element to allow CSS styles such as `stroke-linecap`. This was necessary to match the design. |
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.
Could we maybe clarify that all of the div markup changed to svg and circle elements? Coming back to this, I read it like it was just the fill that changed to SVG until I got to the diffs below.
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.
Oh! and since the sizing classes changed from --small
/--medium
/--large
, can we call that out here too? Something to the effect of "we've deprecated the --small
/--medium
/--large
sizing classes in favor of t-shirt classes --sizeS
/--sizeM
/--sizeL
?
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.
Uh oh- this is actually the progress bar file. We should probably revert these changes.
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's actually a bug in the spectrum-two
branch:
isStaticWhite, |
The progress bar's arg isStaticWhite
should actually be staticColor
. Regardless, I think these changes with the indeterminate group and template should be removed.
@@ -1,6 +1,6 @@ | |||
import { Sizes } from "@spectrum-css/preview/decorators"; | |||
import { disableDefaultModes } from "@spectrum-css/preview/modes"; | |||
import { isIndeterminate, size, staticColor } from "@spectrum-css/preview/types"; | |||
import { isIndeterminate, size } from "@spectrum-css/preview/types"; |
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.
Let's get the staticColor
import back! Yay!
import { isIndeterminate, size, staticColor } from "@spectrum-css/preview/types";
That means we can basically revert these staticColor
control changes as well. In the controls, we should just be able to do staticColor
, since we need both the black and white options now too. 👍
export const StaticBlackDeterminate = Template.bind({}); | ||
StaticBlackDeterminate.tags = ["!dev"]; | ||
StaticBlackDeterminate.storyName = "Static black, default"; | ||
StaticBlackDeterminate.args = { | ||
staticColor: "black", | ||
isIndeterminate: false, | ||
}; | ||
StaticBlackDeterminate.parameters = { | ||
chromatic: { disableSnapshot: true }, | ||
}; | ||
|
||
export const StaticBlackIndeterminate = Template.bind({}); | ||
StaticBlackIndeterminate.tags = ["!dev"]; | ||
StaticBlackIndeterminate.storyName = "Static black, indeterminate"; | ||
StaticBlackIndeterminate.args = { | ||
staticColor: "black", | ||
isIndeterminate: true, | ||
}; | ||
StaticBlackIndeterminate.parameters = { | ||
chromatic: { disableSnapshot: true }, | ||
}; |
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.
Now that we do have the Sizing()
function, what do you tihnk about using it for both of these static black stories? Instead of binding to Template()
, bind to Sizing()
like the static white stories?
export const StaticBlackDeterminate = Template.bind({}); | |
StaticBlackDeterminate.tags = ["!dev"]; | |
StaticBlackDeterminate.storyName = "Static black, default"; | |
StaticBlackDeterminate.args = { | |
staticColor: "black", | |
isIndeterminate: false, | |
}; | |
StaticBlackDeterminate.parameters = { | |
chromatic: { disableSnapshot: true }, | |
}; | |
export const StaticBlackIndeterminate = Template.bind({}); | |
StaticBlackIndeterminate.tags = ["!dev"]; | |
StaticBlackIndeterminate.storyName = "Static black, indeterminate"; | |
StaticBlackIndeterminate.args = { | |
staticColor: "black", | |
isIndeterminate: true, | |
}; | |
StaticBlackIndeterminate.parameters = { | |
chromatic: { disableSnapshot: true }, | |
}; | |
export const StaticBlackDeterminate = Sizing.bind({}); | |
StaticBlackDeterminate.tags = ["!dev"]; | |
StaticBlackDeterminate.storyName = "Static black, default"; | |
StaticBlackDeterminate.args = { | |
staticColor: "black", | |
isIndeterminate: false, | |
}; | |
StaticBlackDeterminate.parameters = { | |
chromatic: { disableSnapshot: true }, | |
}; | |
export const StaticBlackIndeterminate = Sizing.bind({}); | |
StaticBlackIndeterminate.tags = ["!dev"]; | |
StaticBlackIndeterminate.storyName = "Static black, indeterminate"; | |
StaticBlackIndeterminate.args = { | |
staticColor: "black", | |
isIndeterminate: true, | |
}; | |
StaticBlackIndeterminate.parameters = { | |
chromatic: { disableSnapshot: true }, | |
}; |
There's nothing wrong with what you have, it's just the odd ones out of the bunch on the docs 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.
Similar to my progress bar comment, I think we should revert these changes completely. As far as I can tell, they shouldn't be related to your progress circle work, and there's a separate bug with tooltip (not having the variant = "neutral",
arg in it's template).
export const ProgressCircleGroup = (args) => html` | ||
${window.isChromatic() ? html` | ||
${Template(args)} | ||
${Template({ | ||
...args, | ||
isIndeterminate: true, | ||
})} | ||
` : Template(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.
We have the test files after the spectrum-two
update, so we don't even need this anymore! 🥳🥳🥳🥳
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 went through the CSS this morning and things are looking really good! I found we were missing the track-color
token, but overall, I really love all the animation code we were able to remove! Nice work 😄
I did have a question about the animation (and this is a real nit-pick, so feel free to disregard if you are happy with the fill animation!). Is it "stopping" at the top of the progress circle just before the animation loops? I can't tell if that's my eyes playing tricks on me or if there's really a pause.
Screen.Recording.2025-01-09.at.9.15.35.AM.mov
Other than that, my final questions are really regarding high contrast mode.
@@ -33,6 +48,7 @@ export default { | |||
}, | |||
packageJson, | |||
metadata, | |||
layout: "centered" |
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 this is really doing anything anymore. Looks like it's our default now in preview.js, so we can probably delete this line.
components/progresscircle/index.css
Outdated
Copyright 2023 Adobe. All rights reserved. | ||
This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. You may obtain a copy | ||
of the License at http://www.apache.org/licenses/LICENSE-2.0 |
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 just something I've noticed in some of my files. The *
at the beginning of the these lines is gone. I get a warning in my editor. That's not something that blocks this PR or anything, but just something I've seen lately.
We should probably revert these changes anyways, particularly because the copyright year is 2023 in the diff. I think we should have at least the 2024, and probably have it be 2025.
components/progresscircle/index.css
Outdated
/* track double for visibility */ | ||
.spectrum-ProgressCircle-track { | ||
--spectrum-progress-circle-track-border-style: double; | ||
} |
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 need to revisit the high contrast mode. The only reason I started looking into this was because I saw no track for the staticBlack stories. So I pulled it up in AssistivLabs and noticed that there's just some discrepancies where the the staticBlack stories still has its transparent-black track (so you can't really visually see it), the default/sizing/indeterminate stories have a brighter white track (which I think is the gray-200), and then staticWhite has a different track color (the transparent-white token). You can see all 3 different track colors in this screenshot:
I don't really have an answer for this besides asking in the channel.
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.
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.
@marissahuysentruyt I've added the borders for WHCM. For indeterminate mode, the borders are not there. Speaking with design, as of right now the track is not needed for indeterminate just to show the loading state. But for progress the track is necessary.
--spectrum-progress-circle-track-border-style: double; | ||
} | ||
} | ||
@import "animation.css"; | ||
|
||
.spectrum-ProgressCircle { | ||
/* circle unfilled border 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.
…tom properties (#3487) * fix(pickerbutton): add missing custom properties * fix(radio): add missing custom props * fix(calendar): add missing custom properties * fix(stepper): define unused custom props * fix(progressbar): remove unused custom properties; change background-color to background see #2929; fix static color * fix(dial): remove unused custom properties; add undefined custom properties
b438a55
to
46ae687
Compare
Description
S2 migration for progress circle
Changed from using border styles to
stroke-*
styles and attributes. The svg element allows forstroke-linecap
to enable to the rounded path.#3430 Infield progress circle will be sharing this. This component is used in button, picker, and combobox
Animation
@keyframes spectrum-dashoffset-animation
:This animation was created by React Spectrum and used to keep the same speed and bezier curve.
Added tokens
--spectrum-in-field-progress-circle-edge-to-fill
--spectrum-in-field-progress-circle-size-75
--spectrum-in-field-progress-circle-size-100
--spectrum-in-field-progress-circle-size-200
--spectrum-in-field-progress-circle-size-300
Removed styles & mods
--spectrum-progress-circle-track-border-color-over-background
--spectrum-progress-circle-fill-border-color-over-background
--spectrum-progress-circle-track-border-style
--spectrum-progress-circle-track-border-style
--highcontrast-progress-circle-track-border-style
--mod-progress-circle-track-border-style
--spectrum-progress-circle-track-border-style
Validation steps
- [x] Choose static color options to see correct tokens applied
- [x] Toggle indeterminate option see the animation
- [x] Range slider to see the flow of progress track fill
Regression testing
Validate:
Screenshots
To-do list