Skip to content
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: ResourceListDetails doc block #3129

Merged
merged 18 commits into from
Oct 18, 2024

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Sep 18, 2024

Description

In the effort to bring as much of the docs site into Storybook, this PR creates a new <ResourceListDetails /> doc block component. It should replicate this resource section of the docs site:

Screenshot 2024-09-18 at 4 19 12 PM

<ResourceListDetails /> should render each of the resource link cards (<ResourceLinkContent />) if the link exists. To do so, we are pulling data from each component's package.json. A few things should happen with that data:

  • we can get the package name, which corresponds to the package on NPM
  • using that package name, we can programmatically create links to the component's package on NPM and its place in the repo (navigating to the component's README)

There was also a need for a sub-component, <ResourceLinkContent />, to hold the individual resource's content (the SVG icon, the "title" and "subtitle"). There are several styled components as well, mainly used as containers/wrappers, that made up the basis of the styles for the two main components.

Component package.json updates

  1. Adding a spectrum object to each component's package.json (with a guidelinesLink and rootClass keys) gives us the ability to grab the link to the Spectrum Guidelines site. For the majority of components, this single addition is sufficient.
    a. Some package.json files can generate multiple components, such as progress bar and meter. Those components have multiple objects within spectrum, to hold the data for the individual components.
  2. In cases where Spectrum Guidelines do not exist for a component (like accordion or dial), if documentation could be found on the Spectrum Contributions site, the guidelinesLink was pointing to that site instead.
  3. For cases where no guidelines could be found on either of the sites mentioned above, the spectrum array was not added to the package.json. This allows us to just not render that specific guidelines card.

Uncovered issues

The switch and progress bar do not render some of the doc blocks. Neither renders the ComponentDetails or this new ResourceListDetails block. Although both render TaggedReleases, I believe there's issues with the data coming back from NPM for those two components. There is one tag found, at 0.1.0, and that link goes to a deprecated NPM package called undefined.js.

Resolved!

Switch and progress bar were missing some parameters for the package metadata: d88aeba

More details and screenshots regarding this issue can be found in the thread of this message.

Jira/Specs

CSS-772

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

It is necessary to be on the VPN in order to verify and access some of the Spectrum Contributions links.

  • Pull down the branch and run locally with yarn start.
  • Verify each component renders 2-3 new resource cards by visiting their docs storybook pages. You may run into errors (i.e. Quota exceeded.), so there are tips below ⬇️ to resolve that.
  • Some components do not have a Spectrum Guidelines page, but have a page on the Spectrum Contributions site. Verify the following components render all 3 resources cards, and their guidelines card navigates to the proper Spectrum Contributions page.
    • accordion
    • asset card
    • calendar
    • coach indicator
    • date picker
    • dial
    • dialog ("standard dialog")
    • drop zone
    • floating action button
    • illustrated message
    • logic button
    • miller columns
    • opacity checkerboard
    • pagination ("pagination list")
    • steplist
    • stepper ("number field")
    • tag group
    • thumbnail
    • well ("wells")
  • Verify that components with no guidelines only render their GitHub and NPM resource cards.
    • action menu
    • asset
    • asset list
    • clear button
    • color handle
    • drop indicator
    • field group
    • infield button
    • modal
    • picker button
    • split view
    • underlay
  • Some components are "nested" within other components, and both packages are derived from a single package.json.
  • Visit the form docs page. Ensure no guidelines resource card is rendered.
  • Visit the meter docs page. Ensure that a guidelines resource card is rendered and navigates to the meter specific Spectrum Guidelines page.

In your code editor:

  • With the project running, open the .storybook/blocks/ComponentDetails.jsx file
  • In the ResourceListDetails component, change the linkType prop to any value you'd like. (in this example, I used "sparkles")
Screenshot 2024-09-19 at 12 16 07 PM
  • The project should render, and if you look in the browser, you should see a warning message, similar to the one below, catching the invalid linkType value:
Screenshot 2024-09-18 at 1 04 17 PM
  • Revert that change and ensure the project renders as normal.
  • Visit any component docs page (like http://localhost:8080/?path=/docs/components-help-text--docs)
  • Find that component's package.json (/components/your-component/package.json)
  • At the bottom of the package.json, you should see either the spectrum key. Test 3 scenarios:
  1. If the key had a value for componentName, change the value to an empty string.
    • Back in the browser, the guidelines resource card should not render.
Screenshot 2024-09-19 at 12 19 34 PM
  1. If the key had a value, change it to something else.
    • Back in the browser, the guidelines resource card should render. When you click on that card, you should navigate to a URL path with your new value.
  2. If the key was an empty string, set the value to something new.
    • Back in the browser, the guidelines resource card should render. When you click on that card, you should navigate to a URL path with your new value.
  • Verify that form and meter are defined as separate objects in the spectrum array in the field-label and progress-bar package.json files respectively.
  • Check that the cards flow responsively when the window resizes.
Screenshot 2024-09-19 at 1 31 18 PM Screenshot 2024-09-19 at 1 31 09 PM

Things to note

  • Some components don't have a 1:1 match when comparing their name to the title of their Spectrum Guidelines page (i.e. radio => radio group; search => search field). Those were confirmed as the correct link to navigate to.
  • For some components using the Spectrum Contributions site as guidelines, some educated guesses were made. Some components are linked to their default variant (i.e. dialog => standard dialog; pagination => pagination list). Others are linked to a similar name or have a proposed name change (i.e well => wells; stepper => number field).
  • There are 3 components in components/ that are not included in this work: page, site, commons.
  • If you find that the page running locally doesn't render or see a similar Quota exceeded. Failed to execute error in the browser console:
Screenshot 2024-09-13 at 11 29 52 AM

you have to first clear your local storage, then clear your browser cache.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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.

Screenshots

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. ✨

Copy link

changeset-bot bot commented Sep 18, 2024

⚠️ No Changeset found

Latest commit: 5b8bbd5

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

@marissahuysentruyt marissahuysentruyt added the wip This is a work in progress, don't judge. label Sep 18, 2024
Copy link
Contributor

github-actions bot commented Sep 18, 2024

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

Copy link
Contributor

github-actions bot commented Sep 18, 2024

File metrics

Summary

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

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-772-link-doc-blocks branch 5 times, most recently from 82e4159 to 698a73a Compare September 19, 2024 16:46
@@ -55,5 +55,6 @@
],
"publishConfig": {
"access": "public"
}
},
"componentGuidelinesName": ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the docs site, the color handle guidelines link is pointing to the color area guidelines.

Do we want to replicate that link for color handle? Currently, color handle isn't rendering a guidelines card.

@marissahuysentruyt marissahuysentruyt added storybook and removed wip This is a work in progress, don't judge. labels Sep 19, 2024
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-772-link-doc-blocks branch from 174ea48 to 0ac451e Compare September 19, 2024 20:00
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review September 19, 2024 20:01
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I'll keep looking through components to make sure everything looks good, but I'm leaving a review with my questions on the deprecated components!

</DList>
}
</DList>
<ResourceListDetails packageName={packageName} spectrumData={spectrumData} rootClassName={rootClassName}/>
Copy link
Collaborator

@rise-erpelding rise-erpelding Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One very minor thing that I'm noticing is that the GitHub links for the deprecated components don't work as intended. If it's not worthwhile even worrying about that and that was a conversation that already happened somewhere, that's fine!

If not, there are probably several ways we could handle this, and at this point you would probably have the best judgment on which to use, but some thoughts that come to my mind are:

  • Only display the npm link for the deprecated components - either pass down isDeprecated to ResourceListDetails (I don't love this idea), or maybe only conditionally render the GitHub ResourceLinkContent if spectrumData.length !== 0?
  • Don't render ResourceListDetails for the deprecated components at all? So either only render ResourceListDetails if isDeprecated is false, or return early if there's nothing in the spectrumData array (if (!packageName || spectrumData.length === 0) return;)?

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to note now that I've gone through the rest of the components that Action menu has no spectrumData (there's no spectrum key in the package.json), is that deliberate? If so that would have some unintended side effects with these approaches I just mentioned that involve checking the length of spectrumData

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct- it was deliberate to not have a spectrum key for action menu.

.storybook/blocks/ComponentDetails.jsx Show resolved Hide resolved
Comment on lines 390 to 392
href={status ?
`https://github.com/adobe/spectrum-css/tree/main/.storybook/deprecated/${packageName.split('/').pop()}`
: `https://github.com/adobe/spectrum-css/tree/main/components/${packageName.split('/').pop()}`}/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my attempt at trying to replicate the approach we already had in place for the GitHub links. I'm passing the isDeprecated variable, set in ComponentDetails down to ResourceListDetails.

To me, that felt the most simple. When deprecating a component, we already have to go into the story file and add status: { type: "deprecated" } to the parameters. I thought about adding some sort of "repoLink" or "deprecatedRepoLink" to the spectrum data in the deprecated components' package.json's, but that felt like an extra step we didn't really need to add to the deprecation process.

Thoughts?

Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're just about there!

const packageJson = data;

return (
<ResourceSection skipBorder={true} className="sb-unstyled">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that skipBorder={true} was here, do we still need it for anything? It doesn't look like we're using this prop as far as I can see.

@@ -356,7 +356,7 @@ export const ResourceLinkContent = ({ heading, alt, logo, href }) => {
* @param {string} rootClassName - a component's default rootClass arg
* @returns {string}
*/
export const ResourceListDetails = ({ packageName, spectrumData = [], rootClassName }) => {
export const ResourceListDetails = ({ packageName, spectrumData = [], rootClassName, status }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a way to avoid passing another arg here either, now that I know there are other components that don't have spectrumData... so I think this approach works fine! Any objection to changing the name from status to isDeprecated? Since status is a boolean that is only true for deprecated components?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! Just pushed up that change, along with removing that skipBorder. 5376ff8

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-772-link-doc-blocks branch from 5dbb62c to 5376ff8 Compare October 16, 2024 18:44
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-772-link-doc-blocks branch from 5376ff8 to e83a7c6 Compare October 16, 2024 21:07
marissahuysentruyt and others added 18 commits October 18, 2024 09:27
- components that have a valid page on the spectrum guidelines site have
a componentGuidelinesName key now.
- components that do not have a valid page on the spectrum guidelines
site have a componentBetaName key that will point to the
spectrum-contributions site.
- for components that do not any guidelines, setting the
componentGuidelinesName key to an empty string in the package.json will
make sure the guidelines resource card doesn't render.
The ResourceListDetails fetches data from each storybook component's
package.json, specifically utilizing the package name and optional
componentGuidelinesName. It then creates links to the spectrum
guidelines, github, and npm sites. If a component doesn't have a
spectrum guidelines page, it uses the optional componentBetaName and the
spectrum-contributions site instead.

- creates new `styled` components related to the resource section, any
element wrappers and links
- creates ResourceListDetails doc block
- creates ResourceListContent component (which are the individual
resource cards)
- adds some helper functions to generate SVGs corresponding to the links,
switch statements, mapping text, etc.
this applies to form and meter. Form should no longer render a guidelines
link card, and meter should navigate to its own guidelines page instead
of progress bar's.
- update spacing for if() statement
- render ResourceListDetails in ComponentDetails
- console.warn instead of throw an error
- hard codes styles for cards
- extracts SVG code to individual jsx files
- imports those SVG jsx files to use a components
- replaces/refactors svg functions to render new svg components
- creates a `spectrum` key in each component's package.json to use in
ComponentDetails
- `spectrum` array holds metadata for components and/or nested
components, like the componentName, the guidelineLink, and rootClass
- eventually we may be able to add designLink to add Figma links to this
block
- reorganizes file so ResourceLinkContent and ResourceListDetails are
closer to CopmonentDetails, and all fetching/processing helper functions
are together
- addresses missing field label guidelines link
- removes some hardcoded nestedComponent code in favor for a for loop
- indentation fixes
- removed componentName from field label and progress bar package.jsons
- created docsPage parameter for form in the storyMeta
- refactored the nested component for loop to check if the guidelines
are defined, and if docsPage is undefined. If both of those check out,
render the guidelines link. Form is the only component that needed this,
since it's the only nested component that doesn't have a guidelines page
while its parent (field label) does.
To better capture nested components, as well as components that may not
have a guidelines page, this refactor double checks that the hasDocsPage
prop is undefined, as well as that the spectrum.rootClass value includes
a transformed component title. Adding the additional check of the
rootClass helped ensure the proper guidelines link was rendering for
nested components, as well as breaking when the condition was met.
- instead of comparing a story's title (because meter didn't have a
unique rootClass), compare the spectrum.rootClass to the
meta.args.rootClass. This is possible now because we have updated the
default args for meter to be spectrum-Meter.
- removes usage or reformatTitle and hasDocsPage functions and props
- adds more thorough JSDoc comments
- change status prop name to isDeprecated
- remove unnecessary skipBorder prop
@pfulton pfulton force-pushed the marissahuysentruyt/css-772-link-doc-blocks branch from 47b110b to 5b8bbd5 Compare October 18, 2024 13:27
@pfulton pfulton merged commit a5003b6 into main Oct 18, 2024
12 checks passed
@pfulton pfulton deleted the marissahuysentruyt/css-772-link-doc-blocks branch October 18, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge s1 skip_vrt Add to a PR to skip running VRT (but still pass the action) storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants