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

Simplify interaction between canvas and editor #858

Merged
merged 9 commits into from
Aug 30, 2022
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 26, 2022

Simplifying the awkward interaction with the canvas iframe. Moving all logic that requires access to the dom inside the frame to avoid imperatively querying the dom methods in favor for using react refs.

Ultimately the goal is to try and reduce amount of renders for #853 and if that's impossible, lay the basis for extracting a common API to build both preview windows on top of

@apedroferreira we could think a bit more about the architecture for the future.

  • Can we move to a model where we avoid portaling the overlay? instead use the bridge to communicate with the app canvas. e.g. create API's like https://chromedevtools.github.io/devtools-protocol/tot/DOM/#method-highlightNode
  • Would it be possible for such an API to be transferrable? So that we have more options in terms of passing events (window.top.postMessage) and rely less on the awkward handshake to set up the bridge.

@render
Copy link

render bot commented Aug 26, 2022

@oliviertassinari oliviertassinari requested a deployment to bridge-update - toolpad-db PR #858 August 26, 2022 11:08 — with Render Abandoned
@Janpot Janpot changed the title Bridge update Simplify interaction between canvas and editor Aug 26, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2022
@Janpot Janpot marked this pull request as ready for review August 29, 2022 07:11
@apedroferreira
Copy link
Member

apedroferreira commented Aug 29, 2022

Simplifying the awkward interaction with the canvas iframe. Moving all logic that requires access to the dom inside the frame to avoid imperatively querying the dom methods in favor for using react refs.

Ultimately the goal is to try and reduce amount of renders for #853 and if that's impossible, lay the basis for extracting a common API to build both preview windows on top of

@apedroferreira we could think a bit more about the architecture for the future.

  • Can we move to a model where we avoid portaling the overlay? instead use the bridge to communicate with the app canvas. e.g. create API's like https://chromedevtools.github.io/devtools-protocol/tot/DOM/#method-highlightNode
  • Would it be possible for such an API to be transferrable? So that we have more options in terms of passing events (window.top.postMessage) and rely less on the awkward handshake to set up the bridge.

I might not 100% understand the solution we use for this problem and how we use the bridge, or what exactly causes the issue with multiple rerenders, so correct me if i say something wrong. But I understand that what these changes are doing is reducing interaction from the iframe with things outside of the iframe, by keeping as much logic as possible inside the iframe?

If the fact that we are portaling the overlay is adding to the rerendering problem, it should be possible for us to find a different way to make things work. Are you suggesting keeping the overlay outside the iframe, and sending something like events over the bridge to the iframe? Might be simpler and more intuitive than what we're doing right now.
Not sure what you mean by making the API "transferrable", is it something like what I described?

I'd have to try things out with the iframe to see how we could implement this in specific, but the general idea sounds worth exploring.

@apedroferreira
Copy link
Member

I had some wrong assumptions in my previous comment, edited it now, hopefully it's better.

@Janpot
Copy link
Member Author

Janpot commented Aug 30, 2022

I might not 100% understand the solution we use for this problem and how we use the bridge, or what exactly causes the issue with multiple rerenders, so correct me if i say something wrong. But I understand that what these changes are doing is reducing interaction from the iframe with things outside of the iframe, by keeping as much logic as possible inside the iframe?

the communication between host and iframe content currently happens through an object (the bridge) both realms hold a reference to. Setting up this object and making sure both realms are looking at the last correct instance is a bit of an awkward process. Also, a lot of the measuring (getPageViewState) is done by the host realm on a DOM node it queries in the iframe. Setting up this dom node and making sure it stays in sync is a bit awkward. This PR moves the measuring inside of the iframe so that we don't have to manage syncing this dom node as well as the bridge.

@Janpot Janpot merged commit 2a41bf8 into master Aug 30, 2022
@Janpot Janpot deleted the bridge-update branch August 30, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants