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

fix: Storybook 7 viewport menu not working #22829

Merged
merged 3 commits into from
May 30, 2023

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented May 29, 2023

Closes #22718

What I did

Modified getStyles function in code/addons/viewport/src/Tool.tsx so that it works correctly even when prevStyles is undefined.

I did a little bit of digging, and it seems like the bug was introduced by fe02579. The main problem introduced by this commit is that getStyles now always returns undefined if prevStyles is undefined which is initialized as such and never gets updated, so subsequent calls to getStyles in ViewportTool component's body always yield undefined, resulting in no style being applied to the iframe.

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Open Storybook in your browser
  3. Access any story, click the viewport resize button in the preview toolbar, and choose any viewport item in the popover menu, and check if the iframe is resized accordingly.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@kasperpeulen kasperpeulen self-assigned this May 29, 2023
@kasperpeulen kasperpeulen self-requested a review May 30, 2023 07:15
@kasperpeulen kasperpeulen added the ci:daily Run the CI jobs that normally run in the daily job. label May 30, 2023
Comment on lines 115 to 121
if (typeof styles === 'function') {
if (prevStyles) {
result = styles(prevStyles);
}
} else {
result = styles;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

But what now if styles is a function and prevStyles is not yet defined. Then result will also be always undefined right? I think that maybe the right fix would be to rollback what we had before, and change the type to be:

export type Styles = ViewportStyles | ((previous: ViewportStyles | null) => ViewportStyles) | null;

So that the user has to explicitly handle previous being still null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to rollback what we had before

You mean reverting back to before fe02579, right?

And yes you're right the nested if if (prevStyles) was only there because of the type Styles.

And what about the type for prevStyles? The actual value for prevStyles argument comes from const ref = useRef<ViewportStyles>(); which allows it to be undefined, so you either have to give it the initial value or accept undefined for prevStyles arg of getStyles.

Copy link
Contributor

@kasperpeulen kasperpeulen May 30, 2023

Choose a reason for hiding this comment

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

You mean reverting back to before fe02579, right?

Yes!

And yes, widening previous in the type of Styles to undefined instead of null sounds also good to me indeed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey um one more thing about the return type. Since the ref only takes ViewportStyles | undefined I adjusted the return value to be ViewportStyles | undefined rather than ViewportStyles | null. I pushed a revised commit so could you check if everything's alright?

Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

LGTM!

@kasperpeulen kasperpeulen merged commit ca3901c into storybookjs:next May 30, 2023
90 of 91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: viewport bug ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Storybook 7 viewport menu not working
2 participants