-
Notifications
You must be signed in to change notification settings - Fork 4.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
Try a masonry layout on the template pattern suggestion modal #49958
Conversation
Size Change: +5 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
That is a very good idea, James! |
Love it. Both the PR (thanks for excellent before/after demo videos), the code change (so much red), and the net result. Outside of a green check (very eager to hand one out), I feel like we've tried this in the past and reverted it, or am I misremembering? I vaguely recall column-count having some issues with splitting elements in two, or cropping out various pieces. Is there a risk of this here? |
Hmm, not that I'm aware of? 🤔 The same method has been in place in the add-page flow since its inception, and is now also being used in the template part / query modals, so the cat is out of the bag so to speak. If there are issues we'd need to address holistically. I didn't encounter any problems during testing, and I tried with different volumes of patterns. The only thing I couldn't check properly is very large screens. If someone is able to do that it would be appreciated! |
Took it for a spin. Feels good, works well. I think the only open question is whether this is an improvement for the "Start blank" button or not. It's not great in trunk, in the use case you show where there are many patterns. But when there are only very few patterns, it's less successful that it feels arbitrarily smaller: I'm not actually sure what the best solution here is. Perhaps we find a good min-height for "Start blank", and force it to always be the first option? |
I was actually wondering whether we need the dedicated button for starting blank. It doesn't always look great as you already pointed out. It also gets pushed out of vision when there are many patterns. The close button already does the same thing, and feels fairly natural. If that's not adequate on it's own, then perhaps a "Skip" affordance in the modal footer would work better? In any case it's probably worthy of it's own separate exploration.
Good spot, it looks like that appeared as a part of #44028. Imo it would be good for all these previews to have a consistent appearance. Probably good to audit that in a follow-up. |
Given the "Start blank" already exists, we can probably improve it in place. I think it could work in the interim with a few tweaks:
There's just something about portrait that feels more like a template, where landscape feels like a pattern. Just me? |
I think it depends on the template, and the dimensions of things like the header and footer. One thing I've found is that there aren't really rules of thumb for patterns and you almost have to expect the unexpected. That's one of the main motivators for using the masonry layout here – it better suits the wide range of dimensions patterns can exhibit. There's also the question of how prominent the start blank option needs to be. I never find myself drawn to this button. When I'm adding a template and want to start blank, I simply close the modal. |
I don't disagree, and I think there are options. But as is, all the options are wysiwyg, except "start blank", so if we could just get the theme colors in the thumbnail, that would be a decent starting point. |
Thank you for trying that, and a good use of
We may need help on that last one. Not sure if @ntsekouras has bandwidth, or he might know someone who has? I still think "start blank" is useful to have, for someone who isn't as tenured a user as you and me. |
This reverts commit 1444975.
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.
Lots of nice red, happy to try this.
As noted in a comment, I think we need to separately explore improving that "Start blank" button. But since that isn't worse in this branch than what's shipping, it shouldn't be a blocker.
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.
Looks good and the changes are contained in the specific modal for starting a new template, thanks Jay!
Let's land and I'll take a look at the things @jasmussen mentioned in a follow up.
Just saw this on the upcoming dev blog and wanted to chime in on this being a potential accessibility regression. Reflowing the content like this introduces new behavior for keyboard users that tab through the document. In this screencast I am pressing tab to move into the content area, then I press the right arrow key. Screen.Recording.2023-05-08.at.1.44.52.PM.movSince the order of the document no longer represents the visual order of the content, the arrow key right-left movement now moves up-down. As you can see in the video, the jumping from the bottom to the top could also be visually confusing to the user as well, especially if there are very many patterns. There likely isn't a good solution for this unless the mechanism to move left/right gets an upgrade to detect what's visually nearby (I suppose by checking the bounding box), or the masonry functionality is implemented with a JS solution that maintains the document order. |
@KevinBatdorf would you mind opening a new issue describing the problem, just so it doesn't get lost? |
What?
Trying a 'masonry' layout of patterns in the suggestion modal when creating a new template, as an alternative approach to the current layout where all previews are the same size.
Why?
How?
CSS
Testing Instructions
Before
current.mp4
After
after.mp4