docs(popover, progresscircle, radio, rating, sidenav): site docs to storybook

[jawinn]

Description

This continues the migration work to move the documentation from the deprecated docs site to Storybook, covering the components Popover, ProgressCircle, Radio, Rating, and Sidenav. Variants shown on the docs site for these components have been added to Storybook as new stories if they did not already exist. No custom MDX files were needed for these components.

Component specific changes and notes:

• Popover
  • Docs notes have been moved to relevant stories where possible, and many have been reworded to make them easier to understand. All positions are within one Docs story instead of being spread out amongst different examples.
  • Adds test coverage of dialog style popover found in docs (and SWC)
• Radio
  • Separates disabled, read-only, and emphasized options out into their own Docs stories for the user's view, as single radios would not have those options mixed in a group of radios.
  • Also includes a connected fix for Field group, to include a missing role="radiogroup" and to make use of aria-readonly for radios.
• Rating
  • Minor cleanup; adjusts some stories and controls to improve documentation and make sure that any docs site documentation is represented.
  • Fix: Disabled in Storybook was not showing filled stars
  • Fix: Wrong focused class was being applied. Focus ring now shows in story and Chromatic template.
• Sidenav
  • Migrates some of the story descriptions from the docs page, and adjusts story content to clarify what is being displayed
  • Adds additional description to document disabled and selected.
  • Fixes some logic for the display of icons and documents this better. Icons should now appear on all first level nav items when the associated control is true.

Other overall Storybook updates:

• feat: maintain unique name and id args in Sizes() ArgGrid
  Radios displayed with the Sizes() template were no longer grouped and showing a selected radio within in group, because they all had the same name. This addition to the ArgGrid() function makes sure that the id and name args used for attributes have a unique string appended when the template is rendered multiple times within the grid.

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

• Popover - includes all necessary information compared to the docs site
• Radio - includes all necessary information compared to the docs site
• Rating - includes all necessary information compared to the docs site
• Rating fix - disabled shows filled stars
• Sidenav - includes all necessary information compared to the docs site

Regression testing

Some expected VRT changes:

• Additional first level icons appearing in sidenav
• Some updated text in sidenav items
• Fixed display of disabled stars in rating (stars should appear)
• Addition of dialog style content to existing popover Default story

Validate:

1. The documentation pages for at least two other components are still loading, including:

• The pages render correctly, are accessible, and are responsive.

2. If components have been modified, VRTs have been run on this branch:

• VRTs have been run and looked at.
• Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

• I have read the contribution guidelines.
• I have updated relevant storybook stories and templates.
• I have tested these changes in Windows High Contrast mode.
• If my change impacts other components, I have tested to make sure they don't break.
• If my change impacts documentation, I have updated the documentation accordingly.
• ✨ This pull request is ready to merge. ✨

[changeset-bot]

⚠️ No Changeset found

Latest commit: 64cba0e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

File metrics

Summary

Total size: 4.10 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.

[github-actions]

🚀 Deployed on https://pr-2883--spectrum-css.netlify.app

[rise-erpelding]

This was a HUGE effort! I think this is pretty close but I have a few component-specific callouts in addition to the comments I left:

Popover

This is a great improvement! This documentation is really nice and includes all the information from the Docs site, but it works much more nicely and is clearer than the Docs site, plus little things like the vertical transition on open/close work now. ⭐

I occasionally saw a hairline/border on the tip within the tip story (not on Docs), but it wasn't consistent. It would disappear sometimes, and I only saw it on my monitor (not my laptop screen). I didn't notice this with the same story on prod. It might be worth investigating and maybe making a bug card for?

Popover Controls

• There are no controls for nested - this is intentional, right? Would we want to remove controls for the “Positioning options” story too? Do we want to keep that story (with the grid view) in the sidebar? It looks ✨amazing✨ but it keeps crashing my browser tab if I open the story (however it works fine in the Docs page). Maybe keep it on the Docs page and for Chromatic?
• Portions of the "Positioning" options depend on having the tip visible. For instance, without the tip visible, there's no difference between "top-left" and "top-right". This is not a huge deal, but it is a little confusing.

Rating

This was probably just about the first time I’ve ever looked at this component, and I didn’t realize that if there is a rating selected, there’s an indicator only under the selected (and not disabled/read only) rating if the component is hovered. I think the way the docs site has the selected/not selected variants communicates in a useful way, and Storybook would benefit from having some of those selected/unselected variants, for example Default and Default selected, Disabled and Disabled selected.

Side nav

Make sure to use the appropriate variant for the context and user needs. Don’t mix behavior, styles, or variations together in a single navigation menu.

Do you think this is important to retain from the docs site? I didn’t see it communicated in Storybook.

[marissahuysentruyt]

These components are looking much better! All of the additions are much needed and really amp up how useful storybook can be. And I love it all getting into one place. I did find a few things I think need to be tweaked, and then I mainly left you questions. 👏 👏 👏

Really small, random note: want to add progress circle to the title of this PR?

[jawinn]

Thanks for the thorough reviews! I'm almost done resolving all of the comments here, after that big rebase. I'll re-request a review once they're finished.

To your comments Rise:

Popover

I occasionally saw a hairline/border on the tip within the tip story (not on Docs), but it wasn't consistent. It would disappear sometimes, and I only saw it on my monitor (not my laptop screen). I didn't notice this with the same story on prod. It might be worth investigating and maybe making a bug card for?

I'll make a bug card for that. I think it's reproducible if you resize the browser height; it happens on every other (odd?) pixel?

Popover Controls

• There are no controls for nested - this is intentional, right? Would we want to remove controls for the “Positioning options” story too? Do we want to keep that story (with the grid view) in the sidebar? It looks ✨amazing✨ but it keeps crashing my browser tab if I open the story (however it works fine in the Docs page). Maybe keep it on the Docs page and for Chromatic?

I've removed it from the sidebar, since you were having that performance issue. It's probably a lot faster and less problematic now that it's only loading text content and not a Menu in each one, but I think keeping it off the sidebar is probably best.

Rating

This was probably just about the first time I’ve ever looked at this component, and I didn’t realize that if there is a rating selected, there’s an indicator only under the selected (and not disabled/read only) rating if the component is hovered. I think the way the docs site has the selected/not selected variants communicates in a useful way, and Storybook would benefit from having some of those selected/unselected variants, for example Default and Default selected, Disabled and Disabled selected.

I also just realized that the underline not appearing on the star you hover is a feature and not a bug (highlighting the currently selected star value).

This component definitely needs extra coverage for the hovered with underline state, and could have all its stories combined into one Chromatic-only template, though we were trying to exclude that refactoring from this work, which is why I kept this as a light touch. I've added a todo about condensing and increasing coverage. I don't think we need to show empty stars and some filled stars as separate stories as it's a bit duplicative, if you were suggesting that?

Looking at this did surface a bug in our template that I've fixed in the latest commit; disabled was not showing filled stars! Two classes were being excluded for disabled and readonly that shouldn't have been. Also of note, there is an existing bug related to the hovering in the backlog, CSS-735.

Side nav

"Make sure to use the appropriate variant for the context and user needs. Don’t mix behavior, styles, or variations together in a single navigation menu." --- Do you think this is important to retain from the docs site? I didn’t see it communicated in Storybook.

I left this out, as it doesn't seem very helpful and is somewhat inaccurate in our context. For example, it's saying not to mix variations, but you can show headings while also showing icons. Where this sentence originated is already covered with more explanation in the guidelines.

[jawinn]

All of the comments should be addressed now and this is ready for re-review.

Note that I've added todo comments for radio and rating that need followup work for increased VRT coverage and use of the Variants() template.

I'd also like to update this line before merge, once the fix from #2833 is merged in.

[rise-erpelding]

Thanks for working through all the feedback, Josh! This looks great!

To follow up on a couple of things:

• For Popover, I'm going to note here that hairline bug is definitely visible on prod too, so not a bug introduced here as I'd thought!
• Rating looks good with the changes! I can see how having both unselected and selected can be a little duplicative, and I think it's fine not to have both on the Docs page.

[marissahuysentruyt]

I think you took care of all of our previous feedback, so now I just looked through everything with the the modes work & some future work we know is happening in mind! Great work!

[marissahuysentruyt]

Let's get this merged! Thanks for all your hard work on this!

[castastrophe]

A few changes noted in progresscircle and popover. I can review the rest at a later date but I'm seeing a few overlaps there too.

[castastrophe]

Might be a todo for later but it seems like the Dialog content should automatically wrap in the Typography component. If we push it through the renderContent utility exported from @spectrum-css/preview/decorators? We might need to update that function to use Typography styles if the content entry is a string?

[jawinn]

I'm not sure if this completely still applies after the latest set of changes. Let me know if you think there is something that needs updating for this PR.

[jawinn]

I've finished reworking these changes after the rebase with the changes to main. Popover required a bit of work with the stories and templates to get everything displaying again.

• The sizing decorator is no longer used, and the Sizes() utility is used instead.
• I included one small change to the Sizes ArgGrid to help fix the issue with radio buttons having the same name and IDs within the Sizing example. Previously this was causing only one radio to be selected at a time, instead of having one selected within each size group. This also makes sure label for attributes match up with unique IDs.
• I found and fixed another issue with the Sidenav logic around icon. Icons should now appear on all first level nav items when the associated control is true.

[marissahuysentruyt]

This was another beast of a rebase!! Thanks for sticking with this one! I did note a couple places where we could probably remove older custom templates we were making in favor of the new utilities that have merged. I have a few more

For sidenav- did we need to move this migration note on the docs site to the changelog or is it already there? (I didn't check!)
"Core token migration work and design updates added a span tag wrapping the link text."

For the position: left and possibly the position: start & end popover tests: should we add wrapperStyles["align-items"] = "center";? The position: right` case has that same line. I think that would fix this view up:

Those are the only 3 that didn't have a specified wrapperStyles["align-items"] bit. And there might be a reason we don't need to add that, but then do we remove the same line from position: right?