-
Notifications
You must be signed in to change notification settings - Fork 204
chore(storybook): add migrated components list #3745
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
base: spectrum-two
Are you sure you want to change the base?
chore(storybook): add migrated components list #3745
Conversation
|
🚀 Deployed on https://pr-3745--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.43 MB* 🎉 No changes detected in any packages * Size is the sum of all main files for packages in the library.* An ASCII character in UTF-8 is 8 bits or 1 byte. |
83e37cf
to
17e141a
Compare
|
||
This is a list of all components that have been fully migrated to Spectrum 2. | ||
|
||
<MigratedComponentsList /> |
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.
😍
package.json
Outdated
@@ -15,7 +15,7 @@ | |||
"scripts": { | |||
"build": "yarn builder tag:component,ui-icons", | |||
"build:docs": "yarn build:preview --output-dir ../dist/", | |||
"build:preview": "yarn report && nx build storybook", | |||
"build:preview": "yarn report && yarn component:data && nx build storybook", |
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 wonder if there's a way to include this task as a storybook report. In the storybook project.json, if we use this script as the report, would it get run automatically with the others? What's the downside of doing that, maybe it's run more than it needs to be?
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 picturing you could run it like nx report storybook
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.
Left you a comment! #3745 (comment)
8425ef1
to
5d64978
Compare
📚 Branch previewPR #3745 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3745/index.html. |
5d64978
to
0bd74e6
Compare
"report": { | ||
"executor": "nx:run-commands", | ||
"options": { | ||
"commands": [ | ||
"node tasks/migrated-component-scanner.js --output=.storybook/data/migrated-components.json" | ||
], | ||
"cwd": "{workspaceRoot}" | ||
} |
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.
@castastrophe I added a report
target to the storybook/project.json. Is that sort of what you were going for?
Right now, we can trigger this manually with yarn nx run .storybook:report
According to Cursor, yes it could run more than it needs to.
you can add it as a dependency for other targets (like "build" or "start"), so it runs automatically before those.
Example, to run it before build:
"build": { ... "dependsOn": ["report", ...] }
Downsides / Considerations
Performance: If you add "report" as a dependency for "build" or "start", it will run every time you build or start Storybook, even if the component list hasn't changed. This could be unnecessary if the migration status doesn't change often.
Redundancy: If the script is slow or does heavy file I/O, it could slow down your workflow.Best Practice: Only add "report" as a dependency if you want the report to always be up-to-date for every build. Otherwise, run it manually or as part of CI.
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.
Does that mean that we'd have to run yarn nx run .storybook:report
in order to get a component added/removed from the migrated-components.json
? I noticed I did have a difficult time trying to remove accordion from the json file after I removed the migrated status/tags but see that running report works (guess I should have read the validation instructions first 😏). I can see us easily forgetting to update the migrated list once we merge a migration if it's not a part of our regular workflows.
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.
Right now, yes. We would have to run this manually. I was also curious about this report should be incorporated.
We could add it as a dependency to the build task I think, but that would/could make it run more. I'm thinking maybe we err on the side of running as part of our CI workflow when we try to merge to spectrum-two
. I wanted to explore if we can make it a GH action that would commit the changes if there are any?
195b688
to
8e250a8
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.
Is this file something we need and/or want to keep committed? At first, this was basically just to verify that I was grabbing the right data, but I'm not sold that it needs to be committed to the repo. Thoughts? 🤷♀️ Maybe we add it to the .gitignore or something? Maybe there's some options to update it occasionally, sort of like renovate PRs? Whenever we deploy the site, we have to make sure this is up-to-date or something? I don't know...
8e250a8
to
45b8911
Compare
- adds a MigratedComponentsList() block to ComponentDetails.jsx. That block gets used in the S2 Migration guide MDX file - adds a migrated-component-scanner.js task to scan the components directory and generate a list of migrated components based on their status (status: type: "migrated") - adds a migrated-components.json file to the .storybook/data directory - updates the package.json scripts to include the new migrated-component-scanner.js task so that it builds and captures the migration status when storybook is started
- captures the component title so it displays with proper capitalization in the list - generate a component url so it can be used to link to component docs
- in .storybook/project.json, a report script generates the migrated components list - yarn nx run .storybook:report can manually run migrated-component- scanner.js - removes component:data sripts from package.json commands
45b8911
to
7fa91f2
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.
Works as expected, but I had some other questions related to the code!
// Dynamic loading as fallback | ||
const migrated = getComponentsByStatus({ statusType: 'migrated' }); | ||
|
||
if (migrated && migrated.length > 0) { | ||
setComponents(migrated); |
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 seems like this part of the code might be unreachable? If I comment out the previous if block to force the fallback to work, I get an error that this getComponentsByStatus can only be used in a Node.js environment.
</ul> | ||
</> | ||
) : ( | ||
<Body>Loading migrated components...</Body> |
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 the fallback to load dynamically doesn't work and we're always pulling from the json file, it might be worth another look to determine if we need a loading state.
|
||
const { statusType } = options; | ||
|
||
return componentUtils.getComponentsByStatus(statusType); |
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 is a tad confusing to have a getComponentsByStatus
here and also in componentUtils/migrated-component-scanner.js.
* @param {string} options.statusType Status type to filter by (e.g., 'migrated') | ||
* @returns {string[]} Array of matching component directory names | ||
*/ | ||
export function getComponentsByStatus(options = {}) { |
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 situation where this getComponentsByStatus
would run outside of the browser? I see that it gets called by the <MigratedComponentsList>
component, which seems like it wouldn't be in a node.js environment?
"report": { | ||
"executor": "nx:run-commands", | ||
"options": { | ||
"commands": [ | ||
"node tasks/migrated-component-scanner.js --output=.storybook/data/migrated-components.json" | ||
], | ||
"cwd": "{workspaceRoot}" | ||
} |
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.
Does that mean that we'd have to run yarn nx run .storybook:report
in order to get a component added/removed from the migrated-components.json
? I noticed I did have a difficult time trying to remove accordion from the json file after I removed the migrated status/tags but see that running report works (guess I should have read the validation instructions first 😏). I can see us easily forgetting to update the migrated list once we merge a migration if it's not a part of our regular workflows.
Description
DISCLAIMER: MOST CODE GENERATED BY CURSOR
report
target that runsmigrated-component-scanner.js
so that it builds and captures the migration statusJira/Specs
CSS-1259
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
yarn nx run .storybook:report
should run themigrated-component-status.js
task. You should get some feedback like this in the terminal: [@rise-erpelding]Don't be alarmed if you see changes in the
migrated-components.json
file- new components merged so that's a good sign that this script is working!Regression testing
Validate:
Screenshots
Screen.Recording.2025-07-25.at.12.41.55.PM.mov
To-do list