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.

Sign up for GitHub

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

Jump to bottom

Storybook: Add mechanism to redirect moved stories #59181

Merged
merged 2 commits into from Feb 26, 2024
Merged

Storybook: Add mechanism to redirect moved stories #59181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c017aee
Storybook: Add mechanism to redirect moved stories
mirka Feb 17, 2024
5725d92
Simplify URL construction
mirka Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/src/radio-group/stories/index.story.tsx
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@ import { useState } from '@wordpress/element';

const meta: Meta< typeof RadioGroup > = {
title: 'Components (Deprecated)/RadioGroup',
id: 'components-radiogroup',
Comment on lines 18 to +19
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.

component: RadioGroup,
// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170
subcomponents: { Radio },
Expand Down
35 changes: 35 additions & 0 deletions storybook/manager-head.html
View file
Open in desktop
@@ -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.

{
from: /\/components-deprecated-/,
to: '/components-',
Comment on lines +5 to +6
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.

},
];

const params = new URLSearchParams( window.location.search );

const matchedRedirect = REDIRECTS.find( ( { from } ) =>
from.test( params.get( 'path' ) )
);

if ( ! matchedRedirect ) {
return;
}

params.set(
'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.

);

const newUrl =
window.location.origin +
window.location.pathname +
'?' +
decodeURIComponent( params.toString() );
mirka marked this conversation as resolved.
Show resolved Hide resolved

window.location.replace( newUrl );
} )();
</script>