-
Notifications
You must be signed in to change notification settings - Fork 37
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
Block theme: Add drag handles to resize pattern preview #650
Conversation
I checked out this PR and it is working pretty well.
I like the fixed height and feel like this should probably be the default behavior.
I don't understand.
Does this depend on how the pattern was made? Seems to be working for me. On this front, I think we can have an outer bound for the width. I don't see a use case for extending the width indefinitely. A couple of small points:
I appreciate the effort on this PR. 🙇 I do however question the value of this feature and wonder if provides enough extra context for users to find it valuable and use it. What if we shipped without it and added it if requested? |
To @StevenDufresne's comment:
Yeah, I think I'm outvoted 🤷🏻 I like being able to see the whole pattern at once.
For example, this pattern is taller than 600px, so it's cut off. But I can't scroll, it only scrolls the full page. If I disable pointerevents.mp4However, this might just be a FF issue, since scrolling seems to work fine with
Ah, I accidentally missed rebasing the PR on the layout updates. In your version the preview is constrained by the wideSize (1160), as you see by the container not actually expanding. If you try on the rebased PR (pushed now), you'll see it like the screen recording.
We do, it's 2400px. Happy to change that though, I just went with an arbitrary large size. Anything over 1200 will cause the issue, though.
I agree that it's not very valuable, and I was trying to simplify the preview by leaving it out. IMO the 3 size toggles provide enough preview capability. To @jasmussen's comment
You can use wp-env like all the other redesign repos, the instructions are in the project readme.
I can try this, but it's strange to me that the click target will be larger— it's still going to feel like you need to hit a tiny line. |
fa2ee17
to
9848925
Compare
9848925
to
e2f58d2
Compare
So I made a fuss on the call about how hard fixing this up would be, but I think coming back to this with fresh eyes and no pressure was the solution 😅 I spent some time noodling on it and I think I've fixed all the previous weird edge cases. On desktop sizes, this works well — grabbing the drag handles can expand up to 1400, down to 320, and the switching between drag and toggles works. drag-handles.mp4Even with keyboard, the toggles work and left/right arrow keys when the handles are focused will resize the view (same as production). drag-handle-keyboard.mp4It does get awkward on small screens/mobile, since you can "drag" wider than the screen, and shrinking down from a big preview only changes the preview's scale. So the handle & cursor no longer match. The production site gets around this by not allowing a larger preview than the current screen size, but I think that's a silly restriction— a user on a phone should be able to preview desktop size. And with the toggles this works fine. Maybe we should hide the handles at some screen size? Also, I haven't tested this with an actual phone or tablet, so I'm not totally sure how the touch interface will work (trying production on my phone though, it doesn't work well, another point to hiding it at small sizes). drag-handles-small.mp4So I'd actually be comfortable merging this before launch after all, provided it passes others' QA checks, and then thinking through the small screen behavior once it's on the preview site — should be enough time before the proposed launch next week. |
I think this would be fine. We can cycle back if/when this feature is requested. |
Nice work! I'd be happy to launch with this, and even hide those handles at small screen sizes. |
It looks very nice ✨ And agree with Joen, we could hide the handlers. |
This helps prevent the cursor from coming out of sync with the drag handle
Still larger than the 1200 preview width, but more realistic than 2400
98f57ef
to
86fff33
Compare
See #642. This updates the
wporg/pattern-view-control
block to include drag handles for resizing the preview. Since the new version of the block uses the Interactivity API, not React components, we can't "just port over" the old functionality, and we can't use the gesture library. I've made it work fairly well with vanilla JS + interactivity directives, but it still seems janky to me.For keyboard users, they can focus on the drag handles, and use the left/right arrow keys to change the preview width. This matches the current behavior.
I did not use the HTML
draggable
API because it created a ghost-button while dragging, and it seemed to not work on thebutton
element.In the design, the toggles are only 4px wide. When testing, I had a lot of trouble targeting that size, so I've changed them to 8px.
pointer-events: none
or there's an overlay, but then we can't scroll inside the window.Screenshots
On a large screen, resize up and then back down.
large-screen.mp4
On small screen, resize from the "mobile toggle" up to large screen, which scales down the preview (but not the frame height).
small-screen.mp4
How to test the changes in this Pull Request: