Unify placeholders #59275
Unify placeholders #59275
Conversation
jasmussen
added
the
[Type] Code Quality
|
Whilst I appreciate the minimalistic visual of this I am converned about the usibility of this. Loosing the clear heading / label for which block the placeholder state is for is a big regression for me as you get lost much more easily. I'm in favor of retaining the title in the placeholder. I also just want to call out that changing the styling of the variation icons will cause any 3rd party variations to look out of place till they are updated. That is not a big deal but may cause an odd interim step for many. |
|
Yep, definitely agree. This PR is a bit in provocation territory, exploring the "what if". I think there are actually pieces to this that are worth bringing forward, mainly that the code is such a tangled and fragmented mess at the moment. But also that the provocation here can hopefully serve as a catalyst to find the right configuration. E.g. integrating the title in a smaller footprint, and darkening the variation icon color, would be a trivial change, and could potentially still accomplish the same. |
|
Quick sketch, which could use better verbiage but still feels more scalable: 
|
jasmussen
force-pushed
the
try/update-placeholders
branch
from
4859190 to
0999ba1
Compare
|
I've updated this one to clean things up, include the label, and make it ready for review. I think this is a good step forward, let me know!
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -1.89 kB (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Seems like a step in the right direction. Do you think we need the 'Skip' link for columns? There's a quirk in the hover interaction – you can cursor over the button, and the background changes. But the icon doesn't change until you cursor further inside the button: hover.mp4 |
I don't, but I kept it for now to keep the PR smallish, there's a good amount of red in the diff already. Good catch on the hover interaction! My instinct is to make the icon itself change color on hover, I'll do that unless anyone objects. |
|
Not for this PR, and I know we discussed it before, but I wonder if the thumbnail 'icons' could be inspiration for the de-selected state for empty groups and columns, so that the UX matches images. For example an empty group could be a box, empty columns could be 3 adjacent boxes. Might work better in the site view?
|
jasmussen
changed the title
jasmussen
force-pushed
the
try/update-placeholders
branch
from
f913c60 to
9415f4e
Compare
There was a problem hiding this comment.
If this PR ships, it will also resolve some of the issues mentioned in #41961.
In order to move this PR forward, I think it is necessary to investigate from two perspectives.
Placeholder as a component
This PR brings major changes to the layout of the component. Below is a screenshot from Storybook.
Before:
After:
In my experience, this component is relatively popular among developers. I think we need to get feedback from @WordPress/gutenberg-components on how much of an impact this change will have.
Placeholder as the initial state of the block
Regarding variation labels, I would recommend displaying them. There should be enough space to display the text.
For example, with the Column block, We won't be able to see the visual proportions of each variation.
Adding visible text to variations is also mentioned in #60217.
packages/block-editor/src/components/block-variation-picker/content.scss
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-picker/content.scss
Outdated
Show resolved
Hide resolved
| <span className="block-editor-block-variation-picker__variation-label"> | ||
| { variation.title } | ||
| </span> |
There was a problem hiding this comment.
I think this is a breaking change in the BlockVariationPicker component. The title prop defined in the block variation will no longer be referenced.
There was a problem hiding this comment.
It'll be referenced if there is no description. As an alternative, would adding a prop to not have title be fine for each variation? They're not always helpful, like in these.
There was a problem hiding this comment.
Perhaps in __experimentalBlockVariationPicker
|
Thank you for the review, I will try and address your points when I have a moment. Also feel free to push directly if you have chnages in the mean time. 🙏 |
richtabor
force-pushed
the
try/update-placeholders
branch
from
80ee9bc to
eb96420
Compare
|
A good step in the right direction, but we do need to also improve when the placeholders are smaller:
|
|
Good notes, thank you for the help! I've still on my radar to bring this home, hopefully I'll have some time tomorrow. |
|
I'm in favor of refining the This component has been around for a long time, but the title label didn't originally exist. Later, the missing label was raised as an issue in #18737, and the label was added in #19789. As mentioned in #18737, the In this PR, can we focus on the following two things?
Personally, I would also like to see labels added to the Group block variations. In other words, rather than displaying hard-coded variation buttons, we simply use the |
There was a problem hiding this comment.
I looked into the parts this PR is related to. I think a few things need to be addressed in order for this PR to ship. Sorry for including so many screenshots 😅
BlockVariationPicker
I'm a little concerned about the button being adjacent to the label when focused. How about adding a margin of around 4px above the label?
Blocks
Embeds Block
The layout is a little broken.
| trunk | This PR |
|---|---|
 |
 |
Also, this margin is probably unnecessary.
gutenberg/packages/block-library/src/embed/editor.scss
Lines 21 to 22 in 72afd4e
Image Block, Video Block
The buttons are centered. This is due to the style here. This style is only needed for the Site Logo block and Featured Image block, so I think it's better to define it as an editor style for the individual blocks rather than for the Placehoder component itself.
Post Featured Image Block
In mobile view, the upload button is shifted to the left.
Site Logo Block
The upload button is shifted to the left.
Block Plugins
I've tested some plugins that have a very high number of installs, and they don't seem to have a big impact.
Screenshots
JetPack - Form Block
CoBlocks - Carousel
Otter Blocks - Popup
Others
I think these can be done as a follow-up, but we may also want to consider them as the next step.
- Elements with the
components-placeholder__fieldsetclass are always rendered, resulting in unnecessary white space if thePlaceholdercomponent has no children. If there are no children, the element itself should not be rendered.
-
Why hide instructions in the mobile viewport?
-
Several blocks contain CSS that overrides the default styles of placeholders. Some of these should be able to be removed after this PR is applied.
packages/block-editor/src/components/block-variation-picker/content.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-picker/content.scss
Outdated
Show resolved
Hide resolved
|
Thank you for the in-depth feedback 🙏 — I have pushed fixes and hope I do it justice. At a high level, here it should all be captured:
I would really like to land this because so many opportunities for improvement appear when touching this, but I also would love to keep the PR as focused as possible so as to not balloon. Just one example, here's the Embed block URL: That right margin makes the gap between input and button look wrong. It comes from here: That feels a bit too opinionated. But I also don't want to touch that as part of this PR, ideally, to keep things as on-topic as possible. For that same reason, I kept the Embed improvements a separate commit. It now looks like this:
I used VStack and HStack to keep things simple. Hope that's reasonable. |
|
I wanted to touch on this separately because there's an important nuance.
It is not hidden on the mobile viewport. It is hidden when the placeholder becomes small. So there's a JS based element query at play here that applies This is because Placeholder states can be as small as this:
We are so used to seeing the big wide placeholder patterns that it's tempted to add icons, titles, descriptions, notices, error states, and more. But the fact is that it's increasingly common to have patterns that put these blocks with placeholder states in extremely narrow contexts. Here's one from Twenty Twenty-Four:
That's from this branch, and it's revealing some brokenness that I'm now diving in to fix. But I'm sharing because the fact that blocks with placeholder states can become as broken as they are in trunk, when used in narrow contexts, is the entire and full motivation for why this PR initially removed far more than what we ended up with here. The unfortunate fact is that the Placeholder pattern did not scale as it was initially intended. Test pattern |
|
I fixed an issue with site logo centering, and further polished very narrow contexts. Here's thin columns:
Yes, that's not ideal, but here's trunk:
I do not believe there's a way to address those contexts without either moving items into a modal opened by a button, or more generally moving towards this. The PR should now be ready for any final thoughts. Thanks again for all your outstanding feedback. Thank you! |
There was a problem hiding this comment.
LGTM! I believe it now looks correct in all scenarios.
Regarding blocks with the placeholder form, such as the Embed block, #41961 should continue to improve them.
I don't think there is currently a perfect solution to the problem, which is a very narrow context. But at least this PR is better than the trunk.
|
Any concerns about merging on a Friday? Happy to hold off until Monday if you all prefer! |
|
I think it's okay to merge now 🚀 |
|
Thanks all. I'll look at followups, and will keep monitoring feedback on this PR as well. |
* Draft/Experiment: Explore unified placeholders. * Remove comments. * Update label. * Update hover styles. * Update changelog. * Use 2px consistent icons across group and columns * Use : in variation instructions * Add target for hover * Show title. * Initial layout work. * Improve behavior for narrow contexts. * Polish. * Apply margin to Group as well. * Center the buttons when small. * Center upload, 4px padding. * Address feedback (4px gap, justification) * Update embed block. * Fix site logo centering, improve very small contexts. --------- Co-authored-by: Rich Tabor <hi@richtabor.com>
|
While I do realize the huge effort behind this PR, I have to note that it introduced a regression as the Columns placeholder now uses the 'light gray' color for the icons that was already reported as not accessible for the Group block in #60206 Also, removing visible labels and text is not OK for accessibility, and I would say for general usability as well. |
|
I need to second @afercia's comments. This update has replicated a clear and unambiguous existing accessibility issue, since now both the Group and Columns placeholders fail WCAG 1.4.11. |
What?
Update and unify placeholder styles. This makes the Columns block have visually the same setup as Group does. Also updates Image, Video, Audio and other placeholders, emphasizing compactness so that they scale better to narrow contexts.
Previous iteration of this description
WIP exploration to explore unifying the Placeholder component.
Before:
After:
Testing Instructions for Keyboard
Test blocks that have setup states; group, columns, query loop, image, etc.