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

Bug: ensure src of iframe for a ref stays the same, unless the version changes #21713

Merged
merged 9 commits into from
Mar 24, 2023

Conversation

ndelangen
Copy link
Member

Closes #21499

What I did

I ensured that when rendering iframes we don't change the src attribute of the iframe unless the base-url changes (the version changed)

How to test

  1. Add a ref
  2. Start and load storybook
  3. Go to the composed storybook
  4. Switch between stories

Expect no loaders/spinner and no refreshes

@ndelangen ndelangen self-assigned this Mar 21, 2023
@ndelangen ndelangen added the bug label Mar 21, 2023
@ndelangen ndelangen changed the title ensure src of iframe for a ref stays the same, unless the version changes Bug: ensure src of iframe for a ref stays the same, unless the version changes Mar 21, 2023
@ndelangen ndelangen requested a review from tmeasday March 21, 2023 14:53
@ndelangen
Copy link
Member Author

Thank you for the critical eye @tmeasday
It's super appreciated!

I took another look at the code, tried to work out what the intention really is, and was able to simplify a lot, i think.

Care to take another look?
I verified this works on my machine

I even suspect this will be a little more performant too.

@ndelangen ndelangen requested a review from tmeasday March 22, 2023 12:29
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

I like this as it is more consistent with what we do for the main frame (set it once when we first boot and then never again).

However I'm wondering about losing this logic:
image

If I understand the old code directly, it's saying we need to add iframes for auto-inject refs even before they are selected. Is this the type of ref where we don't have an index.json (ie SSv6) so we need to wait for the SET_STORIES (now SET_INDEX) event in order to render the sidebar?

If so, we'll have a chicken and egg situation if we don't show the frame before the ref is selected (it won't be possible to select the ref because we haven't rendered it yet).

@ndelangen
Copy link
Member Author

Ah yes, you're right, the auto-inject refs should be part of the initial state, I'll add it!

@ndelangen ndelangen requested a review from tmeasday March 23, 2023 11:31
@ndelangen ndelangen requested a review from tmeasday March 23, 2023 14:16
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Yes! I love it @ndelangen -- much cleaner now.

@ndelangen ndelangen merged commit d4565ec into next Mar 24, 2023
@ndelangen ndelangen deleted the norbert/fix-composition-refresh-issue branch March 24, 2023 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Composition - switching stories within a composed storybook shows spinners and reloads
2 participants