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

Style book and global styles revisions: zoomed out mode and device switching doesn't work #65630

Closed
2 tasks done
ramonjd opened this issue Sep 25, 2024 · 17 comments
Closed
2 tasks done
Labels
[Feature] Zoom Out Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@ramonjd
Copy link
Member

ramonjd commented Sep 25, 2024

Description

Because the style book and global style revisions are presented in their own iframes, they're not connected to changes in the canvas width when:

  • activating zoomed out mode
  • toggling between device previews

I think it goes beyond activating zoomed out mode for these iframes. I tried it and it's not a great experience. Every time the iframe renders the width "bounces" in zoomed out mode.

style-book-zoom-out-bug.mp4

Tip

A quick fix would be to deactivate the zoomed out / device tools for style book/global styles revisions.

Beyond that, there is an idea to create an editor instance for previewing global styles. This editor would deactivate a lot of block editor tools that are unnecessary.

The zoomed out / device tools could then be introduced incrementally.

Step-by-step reproduction instructions

  1. Go to the Site Editor
  2. Open up the style book or global styles revisions
  3. Activate zoom mode - observe that zoomed out mode doesn't activate
  4. Toggle the device previews - observe that the canvas width doesn't change

Screenshots, screen recording, code snippet

2024-09-25.12.47.32.mp4

Environment info

  • WordPress 6.6
  • Gutenberg trunk

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Feature] Zoom Out labels Sep 25, 2024
@getdave
Copy link
Contributor

getdave commented Oct 7, 2024

I think we should disable Zoom Out for these as I don't believe they add any value.

What do you think @richtabor @draganescu @MaggieCabrera?

@MaggieCabrera
Copy link
Contributor

I think we should disable Zoom Out for these as I don't believe they add any value.

What do you think @richtabor @draganescu @MaggieCabrera?

Sounds reasonable, we can revisit that decision later on

@richtabor
Copy link
Member

I think we should disable Zoom Out for these as I don't believe they add any value.

Yes, that's fine. Perhaps they can be treated just like the device preview control is disabled for template parts.

@draganescu
Copy link
Contributor

The problem here is that again (like in the case of the inserter, and __experimentalSetIsInserterOpened) we need control to higher level implementation (edit site) from lower level (block editor).

When one clicks zoom out the style preview and stylebook both exit and one gets back to cavas. Maybe that would be jarring and sudden, especially since there wouldn't be a way to get back.

So maybe when style book or styles are in the canvas the control doesn't show. But the canvas mode and editor canvas contaner view are both stuff that only edit site deals with.

I think we should have a sort of getZoomOutAvailability which centralises all the situations where zoom out should not be available and also where the higher implementation which does not work with zoom out can turn it off.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 9, 2024

Looks like we need to disable zoom out on the Styles screen too. See #65991

@getdave
Copy link
Contributor

getdave commented Oct 9, 2024

Is the sectionRootClientId setting passed in either of these scenarios? It could be that the editor package itself does not provide the setting if the current screen does not support zoom out.

We'd need to change the implementation so that if the setting is specifically false (or something specific) then Zoom Out doesn't show or function.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 14, 2024

I managed to get something working using useSlotFills to check the slot fill length:

2024-10-14.11.45.02.mp4

There's a flash when toggling between stylebook and revisions - not sure how to mitigate that just yet. Not sure if it's too sketchy or not, but I'll get a PR up soon and we'll find out 😄

It's a band-aid solution until an alternative delivery mechanism is built to display these surrogate canvas views.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 14, 2024

I managed to get something working using useSlotFills to check the slot fill length ... Not sure if it's too sketchy or not, but I'll get a PR up soon and we'll find out

Actually, on second thought, I don't think it's a good idea to hide the preview and zoom tools based on whether the slot is filled, because the behaviour we want to control is very specific to style book and revisions.

The problem here is that again (like in the case of the inserter, and __experimentalSetIsInserterOpened) we need control to higher level implementation (edit site) from lower level (block editor).

True!

The least worst alternatives I can think of are:

  1. Pass down some more values to the Editor Component via specific editor settings, something like settings.visibleTools={ preview: false, zoom: false }, or whatever var names make more sense.
  2. Add a classname to .edit-site-editor__editor-interface and hide it with CSS 🙃

@draganescu
Copy link
Contributor

Some of the problems in this issue have been solved.

styles-style-book-zoom-out.mp4

I updated the issue to reflect the remaining issue.

@ndiego
Copy link
Member

ndiego commented Oct 14, 2024

Some of the problems in this issue have been solved.

@draganescu based on this, do you think we can remove this issue from the 6.7 project board?

@getdave
Copy link
Contributor

getdave commented Oct 14, 2024

I think the Editor leads need to test the original report (also trying it on Gutenberg trunk) and see if it's still there.

If no one gets here sooner I'll try and do that tomorrow.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 14, 2024

Thank you @draganescu and @getdave 🎉

@getdave
Copy link
Contributor

getdave commented Oct 15, 2024

I tested this with the wp/6.7 branch. My findings:

  • I couldn't replicate the original Issue
  • I cannot toggle Zoom Out in Style Book - possible bug the "toggle" remains active 🤔
  • Device preview and Zoom Out are mutually exclusive
  • Entering Styles will Zoom Out and return to original Zoom state when exited.

I think we can close this out (possibly creating a separate issue for the toggle remaining active).

Do you agree @kevin940726 @ndiego @colorful-tones @draganescu?

Screen.Capture.on.2024-10-15.at.10-32-55.mp4

@ndiego
Copy link
Member

ndiego commented Oct 15, 2024

I had the same testing results as @getdave. There are a few minor issues with Style Book that would be great to fix in a follow-up, but they are low priority in my opinion for 6.7.

Ideally, the Zoom Out and Device Preview buttons should be disabled in the Style Book. Right now, they are clickable but do not really do anything. I did notice that the Zoom Out button toggles the breadcrumb bar at the bottom of the window.

style-book.mp4

@getdave
Copy link
Contributor

getdave commented Oct 15, 2024

I had the same testing results as @getdave.

Let's close this one out 👍

Ideally, the Zoom Out and Device Preview buttons should be disabled in the Style Book. Right now, they are clickable but do not really do anything.

We could follow up with that one as a low priority "nice to have" fix.

I did notice that the Zoom Out button toggles the breadcrumb bar at the bottom of the window.

I believe it's intentional in order to remove "chrome" from the view when zoomed.

@ndiego
Copy link
Member

ndiego commented Oct 15, 2024

I believe it's intentional in order to remove "chrome" from the view when zoomed.

Yeah, but it happens in the Style Book as well when toggling the Zoom Out button. Again, pretty minor.

@ndiego
Copy link
Member

ndiego commented Oct 17, 2024

Let's close this one out 👍

Closing out this issue based on the discussion above, but feel free to reopen if needed.

@ndiego ndiego closed this as completed Oct 17, 2024
@github-project-automation github-project-automation bot moved this from 📥 Todo to ✅ Done in WordPress 6.7 Editor Tasks Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Zoom Out Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

No branches or pull requests

7 participants