Closed (fixed)
Project:
Experience Builder
Version:
0.x-dev
Component:
Page builder
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
22 Aug 2024 at 11:23 UTC
Updated:
21 Oct 2024 at 11:59 UTC
Jump to comment: Most recent, Most recent file


Comments
Comment #2
balintbrewsSome background on this:
When zooming happens, we zoom a
<div>outputted by the<Canvas>component by addingtransform: scale(...)in an inlinestyleattribute. (Note that we also set the same attribute on the same element elsewhere, just search forstyle.transformin the codebase.)The same
<div>also outputs<Preview>><Viewport>where we have the iframe and<Outline>laid out as adjacent elements.<Outline>is where all the annotations and labels live. They all get zoomed, because we scale the<div>which is their ancestor.I had a chance to discuss this with @jessebaker, and his idea is the following:
<style>attribute with thetransform: scale(...)further down the component tree to only zoom the contents of the iframe;<Outline>out of the<Viewport>component and lay it on top of the iframe with absolute positioning — this will require calculating its position;<Outline>component needs to be aware of the current zoom level and calculate the dimensions and positioning of the borders it draws around components accordingly.Comment #4
balintbrewsComment #5
bhuvaneshwar commentedComment #6
bhuvaneshwar commentedUnassigned it for now as per the priority of the issue @soaratul you can pick this as discussed.
Comment #7
utkarsh_33 commentedAssigning it to Atul.
Comment #9
utkarsh_33 commentedSo I'll explain the current state of the MR.
1)
is done and it disables the annotation of the buttons to be zoomed in. Regarding 2 and 3 in #2 I tried moving the<Outline>component out of<ViewPort>component but the problem is that it looses access to the iframe ref.To overcome that issue i tried 2 approaches:-- Use a state variable in the parent state and pass it as a prop to the child which then does all the magic
- I also tried using forwardRef but was not completely able to get that working.
Also there's a question regarding current zoom levels mentioned in the 3 point of #2 that doesn'tuseSyncElementSizehook inside theOutlinecomponent deal with this problem(I might be wrong as i was not able to test this because if failing to pass the iframe ref)?Comment #10
balintbrewsUsing
forwardRefsounds like a good approach. Here is the example from the React docs on how to expose a DOM node to the parent component: https://react.dev/reference/react/forwardRef#exposing-a-dom-node-to-the-.... Is this how you tried making it work? Perhaps you could give it another try and show your code if it's still not working?While continuing the work on this issue, let's keep an eye on #3469677: Remove flickering when preview is being updated, which will soon land with changes that will affect how we need to solve this issue. Until then, I think it's worth doing the exercise of making
forwardRefwork, so you clear that complexity first.Comment #11
utkarsh_33 commentedI'll create a separate MR for using the forwardRef approach.
Comment #12
utkarsh_33 commentedI'll try to add the code that i tried for forwardRef.Before that the problem that i faced with the implementation of forwardRef was that we only want the
irfameReffrom the viewPort so that it can be passed to<Outline>component which is moved out from theViewPortcomponent, do we really need forwardRef for that?Isn't there a better/Easy way to approach this?
Comment #14
utkarsh_33 commentedDescribing the current state of MR using forwardRef:-
What's working:-
- I am able to get the iframeRef for the outline component.The outline is place at a static positions using the hardcoded CSS values for now(which we might change subsiquently).
- The outline changes when we switch between the elements.
- The button's annotation are not getting Extra large when zooming(even though it's not correctly positioned).
What's not working:-
- The preview works only for one preview size, that is because we are using the same iframeRef for both the sm and lg previews. Since the ref is mutable and can only be attached to one element at a time, the Outline and Viewport components are both trying to access the same iframeRef, which may be causing the problem.
- I am not able to understand how do i handle the zooming part of the issue.
And as with what Balint mentioned in #10 does it make sense for us to wait for Remove flickering when preview is being updated to land or is there something more that need to explore or work on?
Comment #16
utkarsh_33 commentedSince we will make changes in
3469672-adjust-XB-annotaion-zoom-using-forwardRefMR so closing the other 2 MR's to avoid confusion.Comment #19
utkarsh_33 commentedUploading the video to make it clear how thing are working until this point.
Comment #20
utkarsh_33 commentedThe Zooming work fine for the
widthandheightcalculation but it is not working correctly for the left and top.Comment #21
jessebaker commentedI’ve been taking a stab at this issue. My brain is melting though, it's late on Friday and I’ve not managed to get it working after putting a few hours into it.
I think the solution is pretty complex but it would go some way towards allowing the refactor I have just proposed in #3475759: Implement a different approach to the iFrame preview interactions - at least laying some groundwork for it.
My theoretical solution is roughly as follows:
We need to be able to have the array of
components that show the site at different viewport sizes rendered inside the component so that they scale when zoomed. But we need to have the (and maybe some other components) rendered outside . However both of those components need to access the correct iFrame element as a ref.- At the top level we need an array of refs - I’m thinking this should be stored in a React Context provider because I don't know if storing refs in Redux is a good idea?
- For each viewport size we need to add a ref to that array probably in or above Canvas.tsx
- For each iFrame we render we need to set the appropriate ref in the array to point to the “current” iframe in iFrameSwapper
THENComment #22
wim leersThis is now the last issue for #3454094: Milestone 0.1.0: Experience Builder Demo! 🥳
Comment #23
effulgentsia commented#21 sounds like a nice robust solution, but also potentially a lot of work. I'm wondering if a quick and dirty solution would be to either:
It's possible that both of the above ideas are bad, or perhaps were already tried and failed, so feel free to ignore them. Just throwing them out there in case one of them might work and isn't too much work to implement.
Comment #24
jessebaker commentedOh wow, yeah portals is a fantastic idea! I blame my Friday evening brain for not thinking of that!
Comment #25
balintbrewsBrilliant ideas! While using portals would be a solid approach, I was thinking we could try the second idea first — scaling the not-to-be-scaled components by the inverse amount —, given that if we can make it work, it's probably easier, and we may do a rewrite anyway if we decide to go with #3475759: Implement a different approach to the iFrame preview interactions.
Comment #27
balintbrewsI have a working MR (!325) for scaling the not-to-be-scaled XB annotations by the inverse of current scale. I think this approach will work!
Here is what's left:
<div>outputted inOutline.tsxhas the inlinestyleattribute that cancels out the scaling when zoom is used. This can go into the test case named "Can zoom the canvas with the Zoom Controls".getBoundingClientRect()returns for the outline's DOM element when zoom level is at 100%, and checking how that changes at different zoom levels.@utkarsh_33, anything you can tackle from the list above as you start your day on Monday would be super helpful. Let's make this happen before DrupalCon! 💪🏻
Comment #28
utkarsh_33 commentedHi @balintbrews, the new implementation seems to work fine, except that Add section/component issue described in #27.2.
I was able to write the test coverages according to what's mentioned in #27.3 and 27.4.
I will try to get the work done related to
"Add section" / "Add component" button's.Comment #29
balintbrewsGreat work on the tests and everything, @utkarsh_33, thank you! I left you some comments on the test code. I also would love us to explore if there's a better way to address the button width issue.
Comment #30
wim leersTested on Tugboat (see Tugboat link at the top of the issue — yay, #3472103: Add Tugboat integration is already helping! :D) — it seems to work great:
I still have concerns about the tests though. See details on MR.
Comment #33
balintbrewsComment #34
wim leersNow that this is fixed, I think #3473222: [test-only] Need to update the position of contextual edit component menu while zooming out/in in the canvas is worth looking at next by the same people 😇