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

Storybook: Add mechanism to redirect moved stories #59181

Merged
merged 2 commits into from Feb 26, 2024
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Feb 19, 2024

Part of #33111

What?

Adds a mechanism to "redirect" outdated Storybook URLs to the new canonical URL.

Why?

With the way we are currently structuring our stories, the sidebar grouping names such as "Components (Experimental)" and "Components (Deprecated)" become part of the story URL. This is bad for permalinks because the URL will change if the grouping of a component changes over time. Now with better labeling systems like sidebar icons and status badges in place, as well as the "Experimental" grouping not being meaningful anymore, we want to move to a structure where all wp-components components have a straightforward, stable ID for permalinks (components-my-component).

This PR lays the groundwork so we can move forward with the ID normalization.

How?

I considered some approaches suggested in the Storybook repo.

The Express middleware seemed clean, but it won't work for us because the Node server only runs when in dev mode, and is irrelevant for static deploys.

So I ended up with a modified version of the manager-head.html approach. Instead of listening for all navigation events (since the Navigation API is not ready for production use), we simply run a IIFE on the initial load of the SPA which immediately redirects if applicable. This should be good enough for us, since we can assume there are no outdated links within the SPA — it's only on the initial page load where a user could be arriving from an outdated link somewhere.

Testing Instructions

Accessing the RadioGroup docs or stories from an outdated URL (e.g. ?path=/docs/components-deprecated-radiogroup--docs) should redirect to their new canonical URL.

@mirka mirka added [Type] Enhancement A suggestion for improvement. Storybook Storybook and its stories for components labels Feb 19, 2024
@mirka mirka self-assigned this Feb 19, 2024
Comment on lines 18 to +19
title: 'Components (Deprecated)/RadioGroup',
id: 'components-radiogroup',
Copy link
Member Author

Choose a reason for hiding this comment

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

The plan is to move all the "Components (Experimental)" components into the main "Components" grouping, while keeping the "Components (Deprecated)" grouping separate so outdated components don't clutter the list.

The id is for overriding the id that will be auto-generated from the title. Although it's a hassle to have to manually set this id, I don't expect us having to do it often, because:

  • Not a lot of components are actually going to be deprecated.
  • When we move all the "Components (Experimental)" components to "Components", they will automatically get the simple ids that we want, so we won't need to specify the ids manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

With #58518 adding the idea of status-* tags, it would be nice if we didn't have to do this at all. Ideally, both the id and title could be auto-generated by some build step, and we should rarely if ever have to do anything except set the component.

id is easy enough to generate from the package and component, and title could equally be based on the package name, any relevant status tag, and the component name.

Comment on lines +5 to +6
from: /\/components-deprecated-/,
to: '/components-',
Copy link
Member Author

Choose a reason for hiding this comment

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

Next step will be to add a components-experimental- redirect rule to this list.

Copy link

github-actions bot commented Feb 19, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka requested a review from a team February 19, 2024 16:04
Copy link
Contributor

@andrewhayward andrewhayward left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Nice 👍

Left some nitpicks and minor suggestions for your consideration.

@@ -0,0 +1,35 @@
<script>
( function redirectIfStoryMoved() {
const REDIRECTS = [
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Seems like this could be declared a single time outside of the function (while still kept private from the global scope in a closure)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like in an additional wrapper function? Not exactly sure what you had in mind here.

Though I do think we should extract this into a separate script file if things get more complicated, or if we want to put more JS in this template part file.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, an additional wrapper function. Fine to leave it as-is, this is quite a nitpick at this level.

'path',
params
.get( 'path' )
.replace( matchedRedirect.from, matchedRedirect.to )
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a check for .from and .to just in case they weren't defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine at this scale. Maybe add some type checking if the list gets longer, but at this moment I only expect two or three entries.

storybook/manager-head.html Outdated Show resolved Hide resolved
@mirka mirka enabled auto-merge (squash) February 26, 2024 16:56
@mirka mirka merged commit 9d35ab5 into trunk Feb 26, 2024
60 of 61 checks passed
@mirka mirka deleted the storybook-redirect branch February 26, 2024 17:56
@github-actions github-actions bot added this to the Gutenberg 17.9 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storybook Storybook and its stories for components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants