-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Conversation
Thank you for the critical eye @tmeasday 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 even suspect this will be a little more performant too. |
There was a problem hiding this 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:
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).
Ah yes, you're right, the auto-inject refs should be part of the initial state, I'll add it! |
There was a problem hiding this 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.
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
Expect no loaders/spinner and no refreshes