-
Notifications
You must be signed in to change notification settings - Fork 198
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(infield-progress-cirlce): new component, sharing tokens from progress circle #3430
base: spectrum-two
Are you sure you want to change the base?
feat(infield-progress-cirlce): new component, sharing tokens from progress circle #3430
Conversation
🦋 Changeset detectedLatest commit: 2730b96 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 |
13e35a1
to
c97dee1
Compare
4d277c7
to
221c244
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 know I left a barrage of comments! This is moving along nicely, most of comments are just clean up after the rebase.
Now that spectrum-two
is up to date, we have all of our test files, so we'll need a brand new infieldprogresscircle.test.js
file and tests written 🥳 We should also update the documentation page with the new stories and docs.
components/infieldprogresscircle/metadata/infieldprogresscircle.yml
Outdated
Show resolved
Hide resolved
...ProgressCircle.args, | ||
}, | ||
parameters: { | ||
...ProgressCircle.parameters |
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.
What do you think about adding the tokens design file?
parameters: {
...ProgressCircle.parameters,
design: {
type: "figma",
url: "https://www.figma.com/design/eoZHKJH9a3LJkHYCGt60Vb/S2-token-specs?node-id=14970-6050",
}
}
That way, the design add-on tab will have the in-field progress circle designs that are maybe more relevant. Is that more valuable than just the regular progress circle desktop designs...I'm not sure. I think most of the other components in s2-foundations-redux
are linked to the desktop design files (not the tokens design files), but I wanted to call it out just in case. 🤷♀️
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 the hope for spectrum-two
is that there are no more themes! Looks like the rest of the components have had their express.css
and spectrum.css
files deleted.
components/infieldprogresscircle/stories/infieldprogresscircle.stories.js
Outdated
Show resolved
Hide resolved
components/infieldprogresscircle/stories/infieldprogresscircle.stories.js
Outdated
Show resolved
Hide resolved
components/infieldprogresscircle/stories/infieldprogresscircle.stories.js
Show resolved
Hide resolved
aa3e105
to
b4c24e8
Compare
🚀 Deployed on https://pr-3430--spectrum-css.netlify.app |
File metricsSummaryTotal size: 2.23 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsinfieldprogresscircle
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
3748245
to
33da0e2
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.
This is getting close- thanks for splitting this off from the other progress circle migration!
My main set of concerns I think is just related to this not being an already-published NPM package. So the ComponentDetails
cards don't render at the top of this page since we can't fetch the data from NPM. There's also no metadata.json in the dist
output for this component. I'm not sure where to start with that unfortunately (it might be a @castastrophe question)
If I could be really picky, I'd like to request a change to the regular progress circle controls. I noticed that the value
control in progresscircle.stories.js
doesn't have maybe all of the usual settings, like name, description, etc. Could we update that so it matches the other controls better? This is the same control in progress bar:
value: {
name: "Percent filled",
type: { name: "number" },
table: {
type: { summary: "number" },
category: "Content",
},
control: { type: "range", min: 0, max: 100,},
if: { arg: "isIndeterminate", truthy: false },
},
It's a little more descriptive for users, categorizesvalue
into the "Content" set of controls, and adds that conditional, so it won't even show for a default story if you wanted to get real fancy.
) | ||
InfieldProgressCircle({ | ||
size: size, | ||
staticColor, |
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.
What's the reason for removing the testId: "progress-circle"
line? Do we no longer need 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.
We can delete both the express.css and spectrum.css theme
files for S2 components! 🥳
"spectrum": [ | ||
{ | ||
"guidelines": "https://spectrum.adobe.com/page/progress-circle", | ||
"rootClass": "spectrum-InfieldProgresscircle", |
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 excellent- thanks for adding this spectrum
data! Looks like we just need to capitalize the first c in "Circle" to match the CSS and root class declarations in the other files.
import { disableDefaultModes } from "@spectrum-css/preview/modes"; | ||
import { size } from "@spectrum-css/preview/types"; | ||
import { default as ProgressCircle } from "@spectrum-css/progresscircle/stories/progresscircle.stories.js"; | ||
import packageJson from "../package.json"; |
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! We need the metadata.json stuff.
import packageJson from "../package.json"; | |
import metadata from "../dist/metadata.json"; | |
import packageJson from "../package.json"; |
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.
Something might be missing somewhere. It doesn't look like infield progress circle is correctly building the metadata.json in its dist
file.
@@ -0,0 +1,43 @@ | |||
{ | |||
"name": "@spectrum-css/infieldprogresscircle", | |||
"version": "0.0.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.
Maybe get a second opinion, but should this version number be 1.0.0? Or does that happen when changesets creates a release for us to merge?
*/ | ||
|
||
.spectrum-InfieldProgressCircle { | ||
--mod-progress-circle-thickness: var(--mod-progress-cirlce-stroke-width, 2px); |
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 2px- I see in Figma that there's a token progress-circle-thickness-small
. Should we be using that instead of the hardcoded 2px?
Also, do we want to introduce a mod for this? Are we creating a mod for a mod? 🙃 I don't know the answer to that question, but is this infield progress circle supposed to just overwrite any of the progress circle mods, including --mod-progress-circle-stroke-width
? Should this line just be --mod-progress-circle-thickness: var(--spectrum-progress-circle-thickness-small)
?
And if not- I caught another misspelling of circle in that --mod-progress-circle-stroke-width
. Those pesky L's and C's 😆
// ********* DOCS ONLY ********* // | ||
|
||
export const Sizing = (args, context) => Sizes({ | ||
Template: 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.
We can just write Template,
here, as opposed to Template: Template
if you want to reduce some repetitiveness.
Same thing for the Indeterminate story.
StaticWhiteDeterminate.storyName = "Static white, default"; | ||
StaticWhiteDeterminate.args = { | ||
staticColor: "white", | ||
isIndeterminate: 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.
Any time we have isIndeterminate: false
, we can just rely on progress circle's default arg, where isIndeterminate
is already false. We don't need to redeclare it!
isIndeterminate: true, | ||
size: "s", | ||
size: size, | ||
customClasses: customProgressCircleClasses, |
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.
Should this be customInfieldProgressCircleClasses
? Is that the intention here?
rootClass = "spectrum-InfieldProgressCircle", | ||
size = "m", | ||
staticColor, | ||
...item |
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.
What's the ...item
for here? Is that referencing the parent component that infield PC would be in, like the picker or button?
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's just to spread the rest of props nested in Progress Circle.
"clean": {}, | ||
"compare": {}, | ||
"format": {}, | ||
"lint": {}, |
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.
"lint": {}, | |
"lint": {}, | |
"report": {}, |
Looking at some other project.jsons, I think we need a "report": {}
line.
de1cc68
to
5a38f63
Compare
Description
The infield progresscirlce is a subcomponent of the progress circle which is used in button, combobox, and picker. It's smaller in block size and has its own tshirt sizing.
Mods
--mod-progress-cirlce-stroke-width
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
- [x] Toggle Intedeterminate mode
- [x] Exists in combobox, button, picker components isLoading states
Regression testing
Validate:
Screenshots
To-do list