Skip to content

Dashboard-frontend: add Storybook#22178

Merged
vraja-pro merged 14 commits intotrunkfrom
503-dashboard-frontend-add-storybook
May 9, 2025
Merged

Dashboard-frontend: add Storybook#22178
vraja-pro merged 14 commits intotrunkfrom
503-dashboard-frontend-add-storybook

Conversation

@igorschoester
Copy link
Copy Markdown
Contributor

@igorschoester igorschoester commented Apr 8, 2025

Context

Summary

This PR can be summarized in the following changelog entry:

  • [@yoast/dashboard-frontend 0.1.0 enhancement] Sets up Storybook with stories for the Organic Sessions widget.
  • [@yoast/dashboard-frontend 0.1.0 enhancement] Exposes internal fetchJson and useFetch tools for creating your own widgets.
  • [@yoast/dashboard-frontend 0.1.0 enhancement] Adds override fetchJson option to the Remote Data Provider.

Relevant technical choices:

  • Installed the Storybook from scratch to prevent incurring tech debt from older version. But still used some config from existing places. I tried to skip parts that are not needed, but something might've slipped through.
    • I had copied and then removed the
  • Had a fight with WP/element and React. Ended up needing to specify the React version in the dev dependencies (as well as leaving it in the peer deps). Or React 19 would be installed and that would not work with the createInterpolateElement from WP/element.
  • Decided to expose the fetchJson in the RemoteDataProvider for an easy override, but ended up not using it and just creating objects instead 😅 -- as that is easier to repeat in each story.
  • I had and removed the override the DocsPage template override. We might need it in the future to remove the __forceInitialArgs we have in an older version (UI library). But not needed now.
  • Changed imports in the tests to point to the package src, my IDE likes it as short as possible I suppose
  • I tried making the stories more interactive, which was a hassle due to the data being whatever the remote returns and thus are not the props directly. I managed to do it (using decorator with useArgs), but talking with Vraja we decided to let it go. Instead making a specific story if we want to showcase a specific set.
  • Screen reader bugfix landed in Fix eccessive width for the Organic session widget #22234

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Fire up the storybook and take a look! (but do the normal yarn install first)
yarn workspace @yoast/dashboard-frontend storybook
  • Worth noting that there is a Tooltip story that does not appear on the docs page. If you go to the individual page, you can check the interactions 😄 -- it should open the tooltip (not totally happy with having to test for the class, but 🤷 )

Quick regression/smoke test

  • Build the whole plugin
  • Have the feature flag enable and Site Kit set up
  • Go to the dashboard
  • Verify the widgets are still like before

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

  • Not really testable as this is tooling (until we decide to publish the storybook like we do with the UI library?)

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes https://github.com/Yoast/reserved-tasks/issues/503

@igorschoester igorschoester added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Apr 8, 2025
@igorschoester igorschoester force-pushed the 503-dashboard-frontend-add-storybook branch from 070a3be to 01a8da9 Compare April 10, 2025 10:04
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 10, 2025

Pull Request Test Coverage Report for Build 281d7bdd8288875811777be8c9b74704a063664d

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.727%

Totals Coverage Status
Change from base Build c1d0c0a2c66bb2375d09561dcf6365fdada240e4: 0.0%
Covered Lines: 13548
Relevant Lines: 23151

💛 - Coveralls

@igorschoester igorschoester marked this pull request as ready for review April 10, 2025 10:19
@github-actions
Copy link
Copy Markdown

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

},
},
settings: {
// Ignore certain Storybook packages to avoid import/named errors.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to stem from version mismatches of @storybook/manager and @storybook/theming across different packages in the monorepo:

@storybook/manager-api is used with version ^7.6.17 in ui-library and keyphrase-suggestions.
@storybook/theming is used with versions ^8.6.12 and ^8.3.6.
The dashboard-frontend package uses @storybook/theming version ^8.6.12, which might not be compatible with the current code.
if you rollback the versions:

yarn add @storybook/manager-pi@^7.6.17 @storybook/theming@^7.6.17 --dev

Eslint will not show an error and you can remove that ignore rule.

Or you can update all the storybook packages in the monorepo to 8.6.12 (it's not enough to update only @storybook/theming.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting. They are not hoisted though, due to the different versions. So if ESLint would pick up the nearest node_modules correctly, this would not be a problem?

Either way, I prefer to ignore the false ESLint rule instead of letting that dictate our Storybook version here or elsewhere.
It is not a matter of these two packages, they go with the rest. So that is a full upgrade or downgrade of Storybook. Quite the task for a ESLint false positive.

Comment thread packages/dashboard-frontend/package.json
Comment thread packages/dashboard-frontend/package.json
Comment thread packages/dashboard-frontend/.storybook/main.js
@github-actions
Copy link
Copy Markdown

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

Might be useful when implementing own providers / widgets
* use src index to import in tests
* move closing to same line as opening call
* prefix ReactNode with React namespace, seems to work better in my IDE
* reorder some imports (automatically)

# Conflicts:
#	packages/dashboard-frontend/src/widgets/organic-sessions/compare.js
#	packages/dashboard-frontend/src/widgets/organic-sessions/daily.js
#	packages/dashboard-frontend/src/widgets/search-ranking-compare/search-ranking-compare-metric.js
Without this there is always a horizontal scroll in the stories
As last commit because that was easier for me to ignore while rebasing
igorschoester and others added 3 commits April 28, 2025 15:04
Leaving out the getRandom, I think the return description says enough
Co-authored-by: Vraja Das <65466507+vraja-pro@users.noreply.github.com>
@igorschoester igorschoester force-pushed the 503-dashboard-frontend-add-storybook branch from bb97fc3 to 129a216 Compare April 28, 2025 13:26
@igorschoester
Copy link
Copy Markdown
Contributor Author

Caution

Rebased on the latest trunk (and removed the outdated trunk merge)

@vraja-pro
Copy link
Copy Markdown
Contributor

CR & AC ✅

@vraja-pro vraja-pro merged commit 1cf1cfe into trunk May 9, 2025
22 checks passed
@vraja-pro vraja-pro deleted the 503-dashboard-frontend-add-storybook branch May 9, 2025 11:26
@vraja-pro vraja-pro added this to the 25.2 milestone May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants