-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Assembler: Allow to zoom out the preview #83034
Conversation
@@ -85,7 +84,6 @@ const PatternLargePreview = ( { | |||
> | |||
{ viewportHeight && ( | |||
<PatternRenderer | |||
key={ device } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miksansegundo Do you remember why we use the device as a key to force rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was introduced in https://github.com/Automattic/wp-calypso/pull/76132/files#r1175185900.
I remember this is for the case of patterns like Link In Bio with min-height: 100vh
because when the device viewport changes the patterns must be re-rendered to calculate their content height.
However, we don't have Link In Bio patterns in the assembler, and that logic was cleaned recently in #81310
We can probably remove it. Let me test it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with min-height: 100vh
and the scrolling is gone thanks to the refactoring.
Screen.Recording.2566-10-18.at.10.22.30.mov
We could remove that key but then the animation won't look good. See this Intro pattern dancing 😆 when switching viewports.
Screen.Recording.2566-10-18.at.10.23.27.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's dancing 😆 But the current behavior is not good either as all patterns become blank due to the key changes. Anyway, I'll revert the change and we can improve the experience later
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~299 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~19823 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~7004 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
It's amazing @arthur791004 🤩 I didn't know that you would follow this direction. Very nice iteration. I'll share just some details to make this component amazing. Figma file: Tv3pYqA3EcRfiXC31IxrXE-fi-2469:34894#585639702 Hover state:Hover.effect.movFocus state:Screen.Recording.2023-10-16.at.10.58.57.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color="#C3C4C7" | ||
trackColor="#50575E" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed this issue but I'm considering how to resolve it. Sometimes the layout re-render immediately so the scrollbar will disappear. However, sometimes it doesn't work, and the scrollbar will be there for a while 🤦♂️ |
@lucasmendes-design As the component is from Gutenberg, I'm unsure whether it's possible to change all details but I can try it tomorrow 🙂 Additionally, do you think the zoom-out behavior of the large preview is expected? Or should we resize the large preview with the outer border as well? |
All good. I tried to better align with the viewport component (Desktop, tablet, and mobile). Where the hover is darker and the line is thinner.
Not sure if I understood correctly. What do you mean by large preview? Without the drawer? |
@arthur791004 @lucasmendes-design WDYT about setting a min (say, 25%) to the slider? I don't see much value allowing users to zoom all this way 😅: |
Would it be possible to calculate programmatically the zoom value required to show all the patterns on the preview? It would be useful to have this value as a zoom preset so we can adjust the zoom automatically for users when selecting styles. Something like what I'm doing in this video to adjust the zoom but do it automatically rather than manually. Screen.Recording.2566-10-18.at.11.26.03.mov |
@lucasmendes-design Sorry for not being clear 🙈 Here are examples
But I just noticed #82296 (comment) so I think the answer is to zoom out the entire preview including the gray border 😆 But the downside is the scrolling area would be limited to the zoomed-out preview (i.e. user can scroll the preview inside the gray border area). |
I love this. If this is possible, let's make the value as the minimum possible zoom-out value. Currently, it looks off if you can zoom out to the extent that the patterns are not legible at all 😅 : Edit: sorry, just saw @taipeicoder 's comment above!
|
f9eb541
to
efc2673
Compare
I made changes below
Here is the latest demo: Screen.Recording.2023-10-18.at.6.17.36.PM.mov |
+1000. |
Another solution could be to hide the action bar when the site preview is too small and keep the same size as for, let's say, 100% - 50%. Let me know your thoughts for this and what it's possible here :) |
Yes, but it's intended as we want to avoid the zoom-out slider overlapping the viewport component on the smaller screen. Do you have any idea on this 🤔
Yes, I think we can disable it if there is no selected pattern 🙂
We discussed this on our sync but we assume people might not try to use the action bar when the resolution is too small. We can investigate how to show the action bar outside the pattern as it's the current behavior of the Editor |
@lucasmendes-design I made changes to keep the size of the action bar the same during zoom-out and changed the position to always stick on the top of the active pattern. WDYT? Screen.Recording.2023-10-19.at.9.49.22.PM.mov |
3275eb0
to
36ff2ec
Compare
Understood. It seems wrong the way it is, we're optimizing for the edge case instead of the 80/90% of users. I think we could consider a better solution for a small screen or when it has a specific screen size.
I would try to have something simpler. The action bar above the patterns is not clear to me =/ Suggestion:
WDYT? |
Yep, but there are some nuances:
In the assembler, we have the hover as our main interaction to show the action bar. So, if you hover and show the action bar outside the pattern, there is just one path to access that with the mouse (Red line in the image below).
+1. This is a good point. I would use this behavior only when users zoom out. Let's say, more or less, 0.6x. |
Yes, but the behavior of the style variation preview is quite similar
Yes, I agree. These are the most different parts. |
@lucasmendes-design I made the following changes
WDYT? Screen.Recording.2023-10-20.at.3.46.06.PM.mov |
It's perfect Arthur. Good iteration here 💯 Thanks for that :) |
Ops haha. I thought this screenshot was not a thing yet. Thanks for sharing :) So, I think our component is pretty much aligned with the Editor. |
Non-blocking. There's an odd behavior with scaling carrying over across device switches. WDYT about resetting zoom whenever the user switches devices? cc: @lucasmendes-design Screen.Capture.on.2023-10-23.at.07-49-08.mp4 |
Yes agree, the left most point on the slide bar should be a reasonable value, as @taipeicoder suggested. |
Nice catch! Addressed this issue via 33b7e4f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUPER SMOOTH ❤️
const [ patternLargePreviewStyle, setPatternLargePreviewStyle ] = useState( { | ||
'--pattern-large-preview-background': backgroundColor, | ||
} as CSSProperties ); | ||
const patternLargePreviewStyle = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: I think useMemo()
here is unnecessary as patternLargePreviewStyle
is not props / dependency of anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the style prop of the ul
element 😆
+1. Good idea. We could add that. |
Related to #82296
Proposed Changes
DeviceSwitcher
useZoomOut
hook to control the zoom-out behaviorassembler-zoom-out.mov
Figma: Tv3pYqA3EcRfiXC31IxrXE-fi-2092%3A14696
Testing Instructions
Pre-merge Checklist