-
Notifications
You must be signed in to change notification settings - Fork 196
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
Conversation
|
🚀 Deployed on https://pr-3129--spectrum-css.netlify.app |
File metricsSummaryTotal 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. |
82e4159
to
698a73a
Compare
components/colorhandle/package.json
Outdated
@@ -55,5 +55,6 @@ | |||
], | |||
"publishConfig": { | |||
"access": "public" | |||
} | |||
}, | |||
"componentGuidelinesName": "" |
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.
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.
174ea48
to
0ac451e
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.
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}/> |
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.
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
toResourceListDetails
(I don't love this idea), or maybe only conditionally render the GitHubResourceLinkContent
ifspectrumData.length !== 0
? - Don't render
ResourceListDetails
for the deprecated components at all? So either only renderResourceListDetails
ifisDeprecated
is false, or return early if there's nothing in thespectrumData
array (if (!packageName || spectrumData.length === 0) return;
)?
What do you think?
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.
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
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.
Correct- it was deliberate to not have a spectrum
key for action menu.
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()}`}/> |
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 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?
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 we're just about there!
const packageJson = data; | ||
|
||
return ( | ||
<ResourceSection skipBorder={true} className="sb-unstyled"> |
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 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 }) => { |
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 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?
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.
sure! Just pushed up that change, along with removing that skipBorder
. 5376ff8
5dbb62c
to
5376ff8
Compare
5376ff8
to
e83a7c6
Compare
- 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
- white space removal
- change status prop name to isDeprecated - remove unnecessary skipBorder prop
47b110b
to
5b8bbd5
Compare
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:<ResourceListDetails />
should render each of the resource link cards (<ResourceLinkContent />
) if the link exists. To do so, we are pulling data from each component'spackage.json
. A few things should happen with that data: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 severalstyled
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
spectrum
object to each component'spackage.json
(with aguidelinesLink
androotClass
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.guidelinesLink
was pointing to that site instead.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 newResourceListDetails
block. Although both renderTaggedReleases
, I believe there's issues with the data coming back from NPM for those two components. There is one tag found, at0.1.0
, and that link goes to a deprecated NPM package calledundefined.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.
yarn start
.Quota exceeded.
), so there are tips below ⬇️ to resolve that.package.json
.In your code editor:
.storybook/blocks/ComponentDetails.jsx
fileResourceListDetails
component, change thelinkType
prop to any value you'd like. (in this example, I used "sparkles")linkType
value:package.json
(/components/your-component/package.json
)package.json
, you should see either thespectrum
key. Test 3 scenarios:componentName
, change the value to an empty string.form
andmeter
are defined as separate objects in thespectrum
array in thefield-label
andprogress-bar
package.json files respectively.Things to note
components/
that are not included in this work:page
,site
,commons
.Quota exceeded. Failed to execute
error in the browser console:you have to first clear your local storage, then clear your browser cache.
Regression testing
Validate:
Screenshots
To-do list