docs(actionbutton): migrate docs to storybook #3102
Conversation
aramos-adobe
requested review from
pfulton,
cdransf,
jawinn and
marissahuysentruyt
|
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-3102--spectrum-css.netlify.app |
There was a problem hiding this comment.
@aramos-adobe This is coming along nicely! For the most part, this is a first pass for me, mainly focused on the docs migrations. I found a couple places where maybe we can move over usage notes/migration notes, and then I had some questions for you. I also tried to share our latest approaches.
I'd like to go through once more, and compare your changes to the S2 action button changes (Melissa's branch and then the PR) and make sure anything relevant got brought over.
Way to jump in on this component- it's pretty complicated and used in a bunch of places!!
| import { disableDefaultModes } from "@spectrum-css/preview/modes"; | ||
| import { isActive, isDisabled, isEmphasized, isFocused, isHovered, isQuiet, isSelected, size, staticColor } from "@spectrum-css/preview/types"; | ||
| import pkgJson from "../package.json"; | ||
| import { ActionButtonGroup, ActionButtons } from "./actionbutton.test.js"; | ||
| import { ActionButtonsWithIconOptions, TreatmentTemplate } from "./template.js"; | ||
|
|
||
| /** | ||
| * The action button component represents an action a user can take. |
There was a problem hiding this comment.
Could we add these usage notes here? unless you think we don't need those for some reason? We might have to dig into them and see if they're like, really old and outdated or something.
If we decide we don't need them on the docs page itself, we've been making sure to move them to the component CHANGELOG file 👍
There was a problem hiding this comment.
OH and these migration guide notes too! https://opensource.adobe.com/spectrum-css/actionbutton.html#migrationguide
With these, I've had to comb through the CHANGELOG to see if they were already added. Sometimes looking the yml file can help narrow down at least when the migration guide was added. I've used the Git blame too to find PRs and version numbers to put the notes in the right spot in the CHANGELOG.
And if these are something we need to keep and have documented, we've been adding them to the this description up at the top of the docs page 👍 Just looking quickly, we probably don't need to document the t-shirt sizes, but we'd want to make sure that note in is the changelog somewhere. It might be a similar situation with the focus ring note. Both of those changes have been around for a while and most if not all the components follow those.
There was a problem hiding this comment.
@aramos-adobe Did we need to put any of the usage notes or migration guide on the docs page? I didn't check to see how much of either section ended up in the CHANGELOG, and you might've already figured out we don't need them in Storybook.
Just wanted to call it out since they're not on the storybook docs page, and I might've missed the resolution. Maybe they could even be dispersed into their story-specific documentation blurbs.
There was a problem hiding this comment.
It looks like there is only one note under the old migration guide that should still be included. I would add this to the hold icon story description: "Because of the way padding is calculated, the hold icon must be placed before the workflow icon in the markup."
The rest of the migration notes would be good to move into the CHANGELOG under the appropriate versions where those notes were added (usually I do a little digging on the git blame in Github to find this).
There was a problem hiding this comment.
@aramos-adobe If you could move the migration guide to the action button CHANGELOG, that'd be great! If/when we stop supporting the static docs site, we don't want to lose all the migration guide stuff you found in the yml files, so that's where we've been moving them (from the yml and into the changelog).
And then if we could add Josh's suggestion above for the hold icon docs, that'd be greeaaatttt 🔥
| label: "Edit", | ||
| iconName: "Edit", |
There was a problem hiding this comment.
There's a sentence on the docs site: "The Quiet Action Button should be used where you previously used the deprecated Tool component" with the quiet example. Do you think we should add that description for this quiet story? I don't have a good grasp on the "Tool" component it references, so that might take a little digging to figure out if it's worth it to call out still.
There was a problem hiding this comment.
@aramos-adobe I went through and compared this PR to the S2 action button migration, trying to see if there was anything we might want to grab from there. I honestly didn't see much! I left a note about some hidden controls for the focus, hover, and active states.
And then the only other change from that S2 branch that I'm unsure about is a conditional to the isEmphasized arg. If a user selects one of the static color variants, the isEmphasized control doesn't show in that case. I'm wondering if we should actually keep things the way they are, and not bring that over. The static colors are supposed to overwrite any of our other styles, including emphasized. Maybe it's handy for users to see that when they have an emphasized action button, but they also want it to be a static color variant, the styles are going to be the static color styles. I'm just thinking out loud at this point!
Last thing I promise! In the action button test file, could we add:
{
testHeading: "Quiet + emphasized",
isEmphasized: true,
isQuiet: true,
},
It's showcased on the docs site, so we should probably cover it in the VRT tests. 🤷♀️ I know you're already connecting with Josh, but I'm also to pair on anything if you need!
Great job on this 🎉 ✨
| import { disableDefaultModes } from "@spectrum-css/preview/modes"; | ||
| import { isActive, isDisabled, isEmphasized, isFocused, isHovered, isQuiet, isSelected, size, staticColor } from "@spectrum-css/preview/types"; | ||
| import pkgJson from "../package.json"; | ||
| import { ActionButtonGroup, ActionButtons } from "./actionbutton.test.js"; | ||
| import { ActionButtonsWithIconOptions, TreatmentTemplate } from "./template.js"; | ||
|
|
||
| /** | ||
| * The action button component represents an action a user can take. |
There was a problem hiding this comment.
@aramos-adobe Did we need to put any of the usage notes or migration guide on the docs page? I didn't check to see how much of either section ended up in the CHANGELOG, and you might've already figured out we don't need them in Storybook.
Just wanted to call it out since they're not on the storybook docs page, and I might've missed the resolution. Maybe they could even be dispersed into their story-specific documentation blurbs.
There was a problem hiding this comment.
This is so close!! There's just a few more really small things to take care and this will be good to merge!
I left a suggestion on rewording something, and I also still see the tabs vs. spacing thing Josh found. Josh also mentioned that most of the migration guide stuff (at the very bottom of the docs page) could be put into the CHANGELOG.
There was a problem hiding this comment.
I think this is in good shape now! I believe everything I mentioned has been taken care, so thank you for all the dedication!
I don't know about you, but I love a good list, so I use those checklists in the PR description and I'll check stuff off, like when I test anything in WHCM, or the bit about adding documentation, running VRT, etc. I know you ran VRTs at one point, but I don't know if we have all of the changes captured for this branch yet. This was the most recent one I could find (I think?) https://www.chromatic.com/build?appId=64762974a45b8bc5ca1705a2&number=3186, and it's missing the 5th button we added. We'll have to run those before this gets merged, but I think it's in a good spot for me to approve!
Thanks for all your hard work!
…phasized in autodocs
castastrophe
force-pushed
the
aramos-adobe/css927-migrate-actionbutton-docs
branch
from
a789855
to
b9b1248
Compare
Description
Migrating
actionbuttondocs to storybook.Added missing variants:
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