-
Notifications
You must be signed in to change notification settings - Fork 194
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
docs(statuslight): docs to storybook migration #3152
base: main
Are you sure you want to change the base?
docs(statuslight): docs to storybook migration #3152
Conversation
|
File metricsSummaryTotal size: 4.31 MB* 🎉 No changes detected in any packages * Size determined by adding together the size of the main file for all packages in the library.* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3152--spectrum-css.netlify.app |
bafef35
to
599c210
Compare
export const Disabled = Template.bind({}); | ||
Disabled.args = { | ||
isDisabled: true, | ||
}; | ||
Disabled.tags = ["!dev"]; | ||
Disabled.parameters = { | ||
chromatic: { disabledSnapshot: 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.
We specifically removed the disabled styles and the disabled variant from the docs page in #1495. There's also no disabled status lights in S2. The only reason I added this is because the guidelines page references a disabled status light state. Perhaps that's out of date?
I have a separate commit for the disabled story and testing for easy removal. If we decide to keep the disabled stuff, I have a draft PR here: #3153 to add the disabled styles back into our CSS.
599c210
to
acb656f
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 great! I added a couple of my non-blocking thoughts/questions below and we'll need to follow up one way or the other with Disabled, but clearly you've done the work on that and will apply whichever version is needed! ⭐
* Status lights should always include a label with text that clearly communicates the kind of status being shown. Color alone is not enough to communicate the status. Do not change the text color to match the dot. | ||
* | ||
* When the text is too long for the horizontal space available, it wraps to form another line. | ||
*/ |
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.
May I ask what your reasoning was for not showing the text wrapping to form another line? I think I made the opposite choice in some work I just did for switch, but I'm on the fence on whether I think that's the best way or whether we specifically need to demonstrate text wrapping.
I notice we're testing status light wrapping in the testing preview, which is definitely good, but I'm not sure that necessarily means we need to show it 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.
I feel like I had a similar conversation with Cassondra about this when I moved the documentation over for another component, but now I'm not sure where that conversation is. In-line alert is like this, where the truncation/wrapping option is only in the test view, and I know I migrated those docs. Maybe that's what I was thinking of? Badge is like this too I see, but then checkbox and radio show wrapping on their docs pages.
I figure at least a call out is good on the docs page. And then people can also test with a long label themselves? I know that's not a very convincing reason/answer!
direction: "column", | ||
content: html`${[ | ||
"accent", | ||
"neutral", |
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 had never noticed before (or maybe I forgot) that the neutral variant is italicized. Clearly that's an intentional design decision, but it's nowhere in the guidelines. Have you ever picked up any historical knowledge about that? I'd love to see some sort of explanation on the Docs page, although I definitely don't think it's a requirement here.
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 have much historical knowledge as to why it was italics, but I do know that, for S2, the italicization is no longer supported.
Note: The neutral variant of the status light label uses default-font-style rather than italics as in S1.
Description
This PR continues migrating documentation from the static docs site to Storybook, with a focus on the status light component. When possible, documentation and/or stories were brought over from the S2 status light migration.
CHANGELOG
Jira/Specs
CSS-939
This PR has no CSS changes, so no changeset is needed.
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
Regression testing
Validate:
Screenshots
To-do list