-
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
feat: ResourceListDetails doc block #3129
base: main
Are you sure you want to change the base?
feat: ResourceListDetails doc block #3129
Conversation
|
🚀 Deployed on https://pr-3129--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.31 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsmodal
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
82e4159
to
698a73a
Compare
const npmSvg = () => { | ||
return ( | ||
<ResourceIconWrapper backgroundColor="rgba(203, 56, 55, 0.1)"> | ||
<svg viewBox="0 0 18 7" focusable="false" aria-hidden="true" aria-label="npm"> | ||
<path fill="#CB3837" d="M0,0h18v6H9v1H5V6H0V0z M1,5h2V2h1v3h1V1H1V5z M6,1v5h2V5h2V1H6z M8,2h1v2H8V2z M11,1v4h2V2h1v3h1V2h1v3h1V1H11z"></path> | ||
<polygon fill="#FFFFFF" points="1,5 3,5 3,2 4,2 4,5 5,5 5,1 1,1 "></polygon> | ||
<path fill="#FFFFFF" d="M6,1v5h2V5h2V1H6z M9,4H8V2h1V4z"></path> | ||
<polygon fill="#FFFFFF" points="11,1 11,5 13,5 13,2 14,2 14,5 15,5 15,2 16,2 16,5 17,5 17,1 "></polygon> | ||
</svg> | ||
</ResourceIconWrapper> | ||
) | ||
}; |
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 am super open to feedback on whether or not all of the SVG-related code here makes sense or could be refactored.
components/alertdialog/package.json
Outdated
@@ -64,5 +64,6 @@ | |||
], | |||
"publishConfig": { | |||
"access": "public" | |||
} | |||
}, | |||
"componentGuidelinesName": "alert-dialog" |
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 link is actually broken on the docs site. It doesn't use the hyphen. There are a few components that are in similar boats, where our docs site link is broken, but the intended page does exist. Would this PR be a place to fix that (which would be yml changes)? That feels like a really low priority, especially if the docs site is going away anyways.
components/assetcard/package.json
Outdated
@@ -53,5 +53,6 @@ | |||
], | |||
"publishConfig": { | |||
"access": "public" | |||
} | |||
}, | |||
"componentBetaName": "asset-card-beta" |
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 asset card guidelines link actually goes to the card guidelines.
There is an asset card beta on spectrum contributions. That is what is used here instead. Any reservations against that?
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.
@@ -53,5 +53,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 in-field button guidelines link actually points to the picker guidelines.
Do we want to replicate that link for in-field button? Currently, in-field button isn't rendering a guidelines card.
components/pickerbutton/package.json
Outdated
@@ -52,5 +52,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 picker button guidelines link actually points to the picker guidelines.
Do we want to replicate that link for picker button? Currently, picker button isn't rendering a guidelines card.
} | ||
|
||
else { | ||
throw new Error(`Are you sure you mean "${linkType}"? Please use a valid link type instead: "package", "repository", "guidelines"`); |
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 really know how we handle errors. I was just trying something, so feel free to tell me to remove/change this!
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.
It's a good message but if we throw the error, it blocks the page from rendering. Can we log it as a console.warn?
`; | ||
|
||
export const ResourceLink = styled.a` | ||
position: relative; |
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 have two questions about the styles for the link cards that I'd like some opinions on.
-
The resource cards on the docs site do have a background color. I believe that's because they're using the
.spectrum-Card
class and styles. I didn't immediately add that because I wasn't sure on the whole light/dark mode dilemma I mentioned, and didn't want to hard code anything yet. But should I specify a background color? (probably white?) -
Most of the font colors & styles are being passed down from the
.spectrum-Heading
and.spectrum-Body
classes, but for some reason the underlay page isn't pulling those in. The styles for.spectrum-Heading
and.spectrum-Body
are undefined specifically for the underlay page. I believe it's the only page that has blue link text, instead of black, and all of the font weights and sizes are inconsistent to the other component pages. To be safe, should I hard code any of those styles? I know underlay is funky component, so maybe it's implemented in Storybook differently than our other components, and it isn't loading the heading and body styles at the correct time? I wasn't sure of another way to fix that page, or the way to debug the missing styles.
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.
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 didn't see this happening on dialog, but I did add styles for the font weight, color, font size, line-height and font-family. 8fed17b
if(linkType === "package") { | ||
// NPM package name and link | ||
packageName = packageJson?.name ?? undefined; | ||
packageLink = (packageName && typeof packageName !== "undefined") ? `https://npmjs.com/${packageName}` : false; | ||
} | ||
|
||
else if (linkType === "repository") { | ||
// repo name and link | ||
packageName = packageJson?.name ? packageJson?.name.split('/').pop() : undefined; | ||
packageLink = (packageName && typeof packageName !== "undefined") ? `https://github.com/adobe/spectrum-css/tree/main/components/${packageName}` : false; | ||
} | ||
|
||
else if (linkType === "guidelines") { | ||
// guidelines site name and link | ||
packageName = packageJson?.componentGuidelinesName ?? undefined; | ||
|
||
// TODO: This may not be a sustainable approach to targeting specific nested components. For example, text area is sort of nested under text field, but we don't surface text area as a separate component, like meter or form. We should probably refactor this to either support nested components more dynamically or potentially un-nest components. | ||
if (nestedComponent === "form") { | ||
packageName = undefined; | ||
} | ||
|
||
if (nestedComponent === "meter") { | ||
packageName = nestedComponent; | ||
} | ||
|
||
packageLink = (packageName && typeof packageName !== "undefined") ? `https://spectrum.adobe.com/page/${packageName}` : false; | ||
|
||
// internal contributions/beta guidelines name and link | ||
packageAltName = (packageName) ? undefined : packageJson?.componentBetaName; | ||
|
||
if (!packageLink) { | ||
packageAltLink = (packageAltName && typeof packageAltName !== "undefined") ? `https://spectrum-contributions.corp.adobe.com/page/${packageAltName}` : false; | ||
packageLink = packageAltLink; | ||
} | ||
} |
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.
It feels like there's probably room to refactor this. I'm not a React expert, but some of it feels repetitive. If anybody has ideas on how to more efficiently get the package name and create the right links, I'm all ears! Any suggestions as to how I could do that? Maybe this is ok?
// TODO: This may not be a sustainable approach to targeting specific nested components. For example, text area is sort of nested under text field, but we don't surface text area as a separate component, like meter or form. We should probably refactor this to either support nested components more dynamically or potentially un-nest components. | ||
if (nestedComponent === "form") { | ||
packageName = undefined; | ||
} | ||
|
||
if (nestedComponent === "meter") { | ||
packageName = nestedComponent; | ||
} |
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.
@pfulton I know we talked on slack about removing the guidelines card for form and meter. I wasn't sure quite sure how to do that, without creating some nestedComponent
key in the package.json anyways.
So right now, these are hardcoded. Form will not render a guidelines card. Meter does render a guidelines card that is pointed to its specific guidelines page (as opposed to progress bar's). Let me know what you think of this.
If this isn't to your preference, let me know. I'll remove the meter guidelines and add a note about it into the story documentation for meter & progress bar like we discussed. (i.e. "Meter is implemented using the Progress bar component. For Meter-specific guidance, please see...")
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 also have a test branch with some extra nesting work on it that fleshes this idea out a little. It's in a very rough-draft state.
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.
Leaving some first-pass comments here, all looks really good! I went through the validation instructions and didn't see anything major. I noticed you updated allll the components that have their own .mdx page to add the Component details, thanks!
Is there a follow-up card to look into the issues with Switch and Progress Bar?
Noting that I did not see any QuotaExceededError
s when testing, and I tested in Chrome (mostly), Firefox, Safari, and Edge.
} | ||
|
||
if (nestedComponent === "meter") { | ||
packageName = nestedComponent; |
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'm wondering if anything should be done about this one but I also know you've tagged Patrick about it already, and you called out that Progress Bar isn't fetching the data from npm properly? I noted that Progress bar and Meter each have their own guidelines pages but it doesn't sound like anything can be done about that currently.
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.
Let me know if something is funky with meter. It should be navigating to its own guidelines page. d6de19b (it should have a defined componentName
and guidelinesLink
but I no longer need to hardcode a catch for it. 🤞)
|
||
// TODO: This may not be a sustainable approach to targeting specific nested components. For example, text area is sort of nested under text field, but we don't surface text area as a separate component, like meter or form. We should probably refactor this to either support nested components more dynamically or potentially un-nest components. | ||
if (nestedComponent === "form") { | ||
packageName = undefined; |
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.
Is there a reason this one isn't linking guidelines for form or fieldlabel, since fieldlabel does have them?
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.
oooo I think this must be an error with my nesting attempt...I'll work on this.
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 believe this is fixed here: d6de19b
`; | ||
|
||
export const ResourceLink = styled.a` | ||
position: relative; |
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.
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 left a few notes here that might need some refactor so I'll hold off reviewing in more detail until we can talk about it more.
<svg focusable="false" aria-hidden="true" aria-label="GitHub" viewBox="0 0 36 36"> | ||
<path fill="rgb(0, 0, 0)" d="M17.988 2a16.291 16.291 0 0 0-5.147 31.747c.814.149 1.111-.354 1.111-.786 0-.386-.014-1.411-.022-2.77-4.531.984-5.487-2.184-5.487-2.184a4.32 4.32 0 0 0-1.809-2.383c-1.479-1.01.112-.99.112-.99a3.42 3.42 0 0 1 2.495 1.679 3.468 3.468 0 0 0 4.741 1.353 3.482 3.482 0 0 1 1.034-2.177C11.4 25.078 7.6 23.68 7.6 17.438a6.3 6.3 0 0 1 1.677-4.371 5.863 5.863 0 0 1 .16-4.311s1.368-.438 4.479 1.67a15.451 15.451 0 0 1 8.157 0c3.109-2.108 4.475-1.67 4.475-1.67a5.857 5.857 0 0 1 .162 4.311 6.286 6.286 0 0 1 1.674 4.371c0 6.258-3.808 7.635-7.437 8.038a3.888 3.888 0 0 1 1.106 3.017c0 2.177-.02 3.934-.02 4.468 0 .436.293.943 1.12.784A16.292 16.292 0 0 0 17.988 2z"></path> | ||
</svg> |
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.
The SVGs can be moved into the assets folder and imported if you want to simplify this doc a bit: import GitHubSVG from "../assets/github.svg"
. Then when using it you can reference it like this: {GitHubSVG}
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 was having some issues at first with this. I was getting the file path to render, the SVG code as a string to render, and finally worked something else out to render the SVGs properly. They're JSX files however, not .svg
, so I'm using them as React components instead. I must have been missing something to get the technique you mentioned to work.
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.
if(linkType === "package") { | ||
// NPM package name and link | ||
packageName = packageJson?.name ?? undefined; | ||
packageLink = (packageName && typeof packageName !== "undefined") ? `https://npmjs.com/${packageName}` : false; |
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.
The NPM and GitHub links make sense to hardcode but I think we should abstract the guidelines and contributions to just a "designLink" and storing them in the package.json. I think there's a lot of great info we could make available from there. Let's store it in a spectrum
section maybe like this:
"spectrum": {
"rootClass": "spectrum-ActionButton",
"designLink": "https://spectrum.adobe.com/page/action-button",
},
Maybe for packages with more than 1 component we could use an array? (@pfulton what do you think?):
"spectrum": [{
"rootClass": "spectrum-Form",
"designLink": "https://spectrum.adobe.com/page/form",
}, {
"rootClass": "spectrum-FieldLabel",
"designLink": "https://spectrum.adobe.com/page/field-label",
}],
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.
Let me know if this is sort of what you're thinking. I refactored what I had in all of the package.json
s to include a spectrum
array like you suggested. I ended up going with a "guidelinesLink" key within each object, so that when we end up adding Figma links, we can use the "designLink" language for that. If that's overkill, tell me to change it! 😆 I figured Figma and "design" just went better together, rather than our guidelines pages and "design."
I also re-tried getting the nested component stuff to work- maybe it's not a very elegant solution, but I do think it works. It was a little hard to test very thoroughly just because of that weird "undefined.js" thing showing up for progress bar, so I only had the field label/form nesting pair to work against.
} | ||
|
||
else { | ||
throw new Error(`Are you sure you mean "${linkType}"? Please use a valid link type instead: "package", "repository", "guidelines"`); |
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.
It's a good message but if we throw the error, it blocks the page from rendering. Can we log it as a console.warn?
- 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
0ac451e
to
6d28aa6
Compare
- 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
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.Additionally, a few helper functions were created to map a particular SVG to the corresponding card based on its
linkType
s prop value, and to convert some subtitle/description text based on the thelinkType
as well.Component package.json updates
componentGuidelinesName
key to each component'spackage.json
(set to the component's page path on the Spectrum Guidelines site) gives us the ability to also create a link to the Spectrum Guidelines site. For the majority of components, this single addition is sufficient.componentBetaName
key was added, set to the component's page path on that site instead.componentGuidelinesName
key was left as an empty string. This allows us to just not render that specific 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
.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
.ResourceListDetails
component orComponentDetails
. It will have incorrect information theTaggedReleases
block.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 thecomponentGuidelinesName
key, orcomponentBetaName
. Test 3 scenarios:form
andmeter
are defined asnestedComponentName
key 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