Overview

The zoom interface has controls for debugging the zoom which was added as part of the PoC. There's now a designed version of this interface ready to be implemented. This issue will also remove the debugging controls that were from the PoC.

Note: there's another issue for improving the mouse panning: #3458535: Middle click + drag doesn't work correctly if middle click is inside preview iframe.

Proposed resolution

Implement the simplified zoom interface.

User interface changes

Smooth zoom with pinch and using the slider!

CommentFileSizeAuthor
#26 pinch zoom example.gif7.17 MBjessebaker
#7 Screenshot 2024-07-26 at 9.08.31 AM.png75.31 KBgauravvvv
#7 Screenshot 2024-07-26 at 9.06.09 AM.png165.16 KBgauravvvv
#4 Screenshot 2024-07-24 at 9.39.18 AM.png39.34 KBgauravvvv
Screenshot 2024-07-23 at 12.49.42 PM.png2.75 KBlauriii
Screenshot 2024-07-23 at 12.51.28 PM.png181.67 KBlauriii

Issue fork experience_builder-3463307

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lauriii created an issue. See original summary.

gauravvvv’s picture

gauravvvv commented
Assigned: Unassigned » gauravvvv

Gauravvvv opened merge request !116

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2024-07-24 at 9.39.18 AM.png39.34 KB

Based on the image in the issue description, the range slider for zoom should not include additional values like X, and Y points or the "debug to scroll" button. Do I need to hide these in the current tray or create a new one for the range slider? Thank you.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Could we add a "debug" prop to the component to show these things? In a future issue, we could consider toggling that based on an environment variable (or something else) but for now it could be something that developers would have to turn on manually within code.

gauravvvv’s picture

gauravvvv commented
Assigned: gauravvvv » Unassigned
Status: Active » Needs review
gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2024-07-26 at 9.06.09 AM.png165.16 KB
new Screenshot 2024-07-26 at 9.08.31 AM.png75.31 KB

I have added a prop debug, if it is true then it will display the X, Y coordinates and "Debug: scroll to middle" button and on debug false it will only display the zoom range slider.

Debug: false

Debug: True

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Related issues: +#3452582: Make store API paths/uris configurable via environment variables (and eventually drupalSettings)

Those screenshots look great, @Gauravvvv! 🤩

Keeping at Needs review, but IMHO this should be integrated with AppConfiguration (introduced by #3452582) — see MR review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Merged in upstream, CI should be green again 👍

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

This looks pretty nice! It would be super cool if we could slightly smoothen the transition when zooming. This can be a follow-up but could be worked on here too in case you're interested in this 😊

gauravvvv’s picture

gauravvvv commented

Addressed feedback on using AppConfiguration as well smoothen the transition when zooming

Gauravvvv changed the visibility of the branch 3463307-zoom-interface to hidden.

Gauravvvv changed the visibility of the branch 3463307-zoom-interface to active.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Unit tests and E2E tests are failing … 😅

gauravvvv’s picture

gauravvvv commented
Assigned: Unassigned » gauravvvv
gauravvvv’s picture

gauravvvv commented
Assigned: gauravvvv » Unassigned
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Still not passing tests … 😅 why are you marking this Needs review? 🤔

gauravvvv’s picture

gauravvvv commented

Hi @Wim Leers, I changed the status to needs review before the test results.

jessebaker made their first commit to this issue’s fork.

jessebaker’s picture

jessebaker commented
Assigned: Unassigned » jessebaker
Status: Needs work » Active
jessebaker’s picture

jessebaker commented
Assigned: jessebaker » bnjmnm
Status: Active » Needs review

This is now ready for review. I picked up from the work by @gauravvvv and implemented the suggestion from @lauriii in #10. Zoom controls now exist in their own React component and an array of tests have been added (and the existing tests cleaned up as a result of me getting absolutely stuck on a weird race condition that seemingly was nothing to do with this issue!)

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Needs review » Needs work

Looking good - got some nits and bits in the MR that I'm sure can be addressed pretty easily.

gauravvvv’s picture

gauravvvv commented
Assigned: Unassigned » gauravvvv
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: gauravvvv » bnjmnm
Status: Needs work » Needs review
Issue tags: +Needs screenshots

Awesome progress here! 👏

Can we get an updated GIF in the issue summary to showcase that awesomeness? 🙏😇

Also: AFAICT this is ready for review! 😄

balintbrews’s picture

balintbrews
he/him/his
Primary language Hungarian
Location Netherlands 🇪🇺
commented

Just adding a comment here to note that there is some testing code added in #3463618: Include component props form in undo (commit in MR: 5aa96e) which should be updated once this issue lands and we have the loadURLandWaitForXBLoaded command available.

jessebaker’s picture

jessebaker commented
Issue summary: View changes
StatusFileSize
new pinch zoom example.gif7.17 MB

hooroomoo made their first commit to this issue’s fork.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Issue summary: View changes
Issue tags: -Needs screenshots

Removed the debugging controls from the PoC and updated the IS accordingly. I don't think there is really a need for them right now and we can easily add them back if that changes in the future.

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Assigned: Unassigned » bnjmnm
Status: Needs work » Needs review

  • bnjmnm committed bd8ee98e on 0.x authored by gauravvvv 
    Issue #3463307 by jessebaker, gauravvvv, Wim Leers, bnjmnm: Implement...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Needs review » Fixed
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

So nice to see this improvement land! 🚀 🎉

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.