Update Navigation block placeholder #27018
Conversation
|
Size Change: +33 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
Before:
After:
This is a massive step in the right direction, but needs some design love. I'm going to push some things to try and get us closer to what Shaun suggests in #23207 (comment). |
jasmussen
force-pushed
the
try/23207-nav-placeholder
branch
from
555ab53 to
de585b8
Compare
|
I pushed some tweaks to:
It now looks like this:
Props to @shaunandrews for the design he created in #23207 (comment). I think it works really well, and I can think of no better way to try this than in the plugin proper. It if works well, I would like us to explore how we can make this skeleton setup state a bit more generic, so it's easier to create on a per block basis. It seems like with a little clever component work, all of the unselected/selected, and the white box with a horizontal row of buttons/dropdowns, can be made pretty easily replicable. A next step for me is to make this setup state aware of whether the navigation block is horizontal or vertical. Stay tuned. |
|
Pushed a fix to make the placeholder state orientation aware:
But I'm not quite feeling it. The shift is rather jarring. I'm going to continue to tinker with this one. |
|
The placeholder state with semi transparent currentColor works well in other-color themes, btw:
|
|
Alright, I think it's in a pretty good place now:
I would appreciate eyes on the code, and good testing! |
|
Cc @youknowriad @gziolo @karmatosed, I'd appreciate code and design reviews on this as some of our american friends are out for a well deserved break. |
|
Edit: Ignore my comment, I saw screencasts shared by @jasmussen :) |
There was a problem hiding this comment.
There are all e2e tests removed related to this block. I would defer to @getdave and @talldan who co-authored them to decide whether it is possible to refactor them to work with the current workflow. It definitely would be beneficial to have user-facing tests to avoid future regressions. Other than that, I don't have any concerns regarding the proposed changes. Nice improvement 💯
mkaz
force-pushed
the
try/23207-nav-placeholder
branch
from
a97e8f2 to
bca5c9a
Compare
|
Most of the existing end to end tests were for the placeholder, so I can understand they'd need some updating. The new placeholder seems pretty much the same in functionality but different (and a nice improvement) in presentation, so it feels like the tests still have validity. In the commit description that removed the tests:
@mkaz My concern with removing the whole file is that while it's easy to bring back, it might just be forgotten, and there's some handy code in the file for mocking the endpoints that will be very useful. I reckon it'd be nice to leave in place something simpler and edit: Just noticed the eslint rule is only a warning—probably a reminder not to leave a test skipped in the long term, so probably ok to skip anything challenging to update. |
TIL 😀 |
mkaz
force-pushed
the
try/23207-nav-placeholder
branch
from
bca5c9a to
a973035
Compare
- Remove standard Placeholder - Break apart menu and create new as individual actions - Move all pages to own action
mkaz
force-pushed
the
try/23207-nav-placeholder
branch
from
a973035 to
3ebc40e
Compare
Description
Switch larger standard placeholder to inline menu that fits in more places. This updates the overall placeholder when the block is first inserted. There is a related PR here that updates the empty state when a menu is inserted #26947
Fixes #23207
How has this been tested?
Types of changes
TODO