Persistent iframe - #23
Conversation
rusty-key
left a comment
rusty-key
left a comment
There was a problem hiding this comment.
Sorry about some unrelated formatting changes, looks like refmt defaults updated at some point 🤷♂️
Besides main refactor, I had to tweak right sidebar because it was throwing at runtime with my solution.
| | None => React.null | ||
| | Some(body) => ReactDOM.createPortal(children, body) | ||
| }} | ||
| </iframe> |
There was a problem hiding this comment.
This is the core change. Instead of referencing iframe with demo, we just render demo using portal in persistent iframe
There was a problem hiding this comment.
Have you thought about how this change might affect stateful changes that the iframe keeps from one demo to another? E.g. if some iframe loads some styles in the page head then they might "leak" from one demo to the next.
There was a problem hiding this comment.
No, I didn't. I assumed this is the responsibility of the consumer: do a cleanup on unmount.
| | Some(window) when !showRightSidebar => | ||
| Window.postMessage(window, RightSidebarDisplayed) |
There was a problem hiding this comment.
I am not sure why it's done via "signalling", but decided to keep it this way for now to limit the scope of this PR
| <div name="DemoUnit" style=Styles.container> | ||
| <div style=Styles.contents> {demoUnit(props)} </div> | ||
| {switch (parentWindowRightSidebarElem) { | ||
| {switch (sidebarElem) { |
There was a problem hiding this comment.
I also refactored the sidebar to use reference instead of getting elements by id, which seems much more fragile and caused runtime exceptions for me.
|
I think it's ready for review. I am not sure why things are implemented via signalling instead of using callbacks, so I refactored relevant part and willing to refactor more in the future. But please let me know if I am missing something. |
|
oh well, there is an issue with incorrect mounting of styles/scripts. They get mounted to parent root instead of iframe :/ |
jchavarri
left a comment
jchavarri
left a comment
There was a problem hiding this comment.
I'm not sure how styles don't "leak" from one demo to the next (see inline comment).
| let useFullframeUrl: bool = [%mel.raw | ||
| {js|typeof USE_FULL_IFRAME_URL === "boolean" ? USE_FULL_IFRAME_URL : false|js} | ||
| ]; |
There was a problem hiding this comment.
my bad. I was not sure what it was doing and how to debug it. Will check
| | None => React.null | ||
| | Some(body) => ReactDOM.createPortal(children, body) | ||
| }} | ||
| </iframe> |
There was a problem hiding this comment.
Have you thought about how this change might affect stateful changes that the iframe keeps from one demo to another? E.g. if some iframe loads some styles in the page head then they might "leak" from one demo to the next.
2 participants
Currently, reshowcase is reloading
iframeevery time we go from demo to demo. This causes fetching/parsing/executing all assets:https://github.com/ahrefs/reshowcase/assets/1197026/e6705a68-2cd9-4c3a-8ad6-4122e77e38e8
It makes reshowcase navigation unnecessarily slow, especially on large applications.
Instead, we can make iframe persistent, and only replace its content. This way, reshowcase will fetch assets only once and only need to render relevant demo on navigation:
https://github.com/ahrefs/reshowcase/assets/1197026/798529fc-44db-4bea-8018-87794116c829
This is not yet clean solution, just "showcase" of possibility. I'll continue working on it