Navigation: Remove blobs that look like a loading state #37099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
jasmussen
changed the title
|
Size Change: -382 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
priethor
reviewed
Dec 3, 2021
packages/block-library/src/navigation/edit/placeholder/placeholder-preview.js
Outdated
Show resolved
Hide resolved
|
Oh that's a bug, let me fix that real quick. |
|
Thanks Hector! @annezazu I know you reported many of the problems with the blobs in the past. Do you have any thoughts to share? Thank you for your time! |
|
Thanks for tagging me into this! Compared to the other exploration, I like the consistency and simplicity this provides. I think it will be important for these placeholder states to be thought of cohesively so am in favor of this exploration over on that basis alone: #36858 For great context and thoughts... Just this week, I got feedback from someone in the current call for testing around how they expected to see the three blobs on the front end of their site but I haven't heard this as widespread feedback. If anything, my main concern would be around ensuring that the placeholder makes it clear that it's both related to a menu and that action needs to be taken to configure it, especially since compared to the post featured image one does need to take action (vs the featured image just being pulled in). In looking at both the post featured image and navigation block placeholders together, my brain immediately thinks about how well these placeholders will scale if they all look so very empty. In their current state, it's hard to see at a glance what each of these items are without requiring folks to click into them. I wonder what else could be included instead of just the icon for the navigation block in the placeholder? I don't know if that's going to be enough to differentiate compared to what is in place for featured image:
vs
The former really gives more insight at a glance as to what each placeholder represents IMO and I wonder what will be lost by removing some indication that it's for the nav block. Perhaps keep the name in there?
|
|
Excellent thoughts, thank you all. If there's one take-away from parsing this feedback over the weekend, it only affirms that the blobs don't quite work. I think this dashed version is probably better, but maybe it isn't that much better. It might need more design thought. And so I think it boils down to the question: do we want to land this for 5.9 and then continue to iterate, or do we want to go with the blobs? It's very easy to get caught up in the moment of wanting to pack as much as possible into the release. At the same time, there are more important fixes to make, and there will be the plugin to ship fixes in, and 6.0 a few months later. I'll keep thinking and see if there's a way to thread the needle between looking like navigation and looking like it needs further action. |
This was referenced Dec 6, 2021
7 tasks
38 tasks
jasmussen
force-pushed
the
try/navigation-alt-setup-state
branch
3 times, most recently
from
8125064
to
a8e0dc5
Compare
jasmussen
force-pushed
the
try/navigation-alt-setup-state
branch
from
a8e0dc5
to
243f776
Compare
|
I've rebased, refreshed, and updated this one based on feedback. I've also included a new description with a fresh GIF of how it looks. I think with the dashed lines, and the icon plus label, this placeholder state well balances the need to clearly look like a placeholder, but at the same time look unconfigured. |
priethor
reviewed
Jan 26, 2022
|
Ack, and I had it fixed before even! Here it is again, thank you: |
getdave
reviewed
Jan 27, 2022
There was a problem hiding this comment.
Thank you for pursing this 👏
I greatly prefer this state to the blobs. It's much more visual "stable" and it's relatively clear that the Navigation is in an unconfigured state.
I found another bug with the colors on a dark background. I believe you've fixed the placeholder preview but not the placeholder component itself.
Screen.Capture.on.2022-01-27.at.10-22-08.mov
Code to fix is here:
gutenberg/packages/block-library/src/navigation/edit/placeholder/index.js
Lines 160 to 163 in b3e57fb
This points to redundancy in the code which could be refactored to avoid the need to update in x2 places. Maybe one to add to the list for #37190
|
What an excellent review, thank you. I'll get on that! |
|
There's definitely room to refactor and remove a ton of stuff. In this case I think the recently pushed change fixes it right up. If I'm right, the color-inheritance rule was to blame as it was attached to ☝️ that used to be broken, just like it was in your screen recording. |
getdave
approved these changes
Jan 27, 2022
|
Thank you for the reviews and help! |
38 tasks
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Alternative to #36858 and #37144. Redesigns the navigation block setup state to not show fake menu item skeleton indicators.
The primary problem to solve is to make the navigation block look unconfigured, but still in this state blend a little bit into the theme to provide a better initial experience. The problem with the gray skeleton indicators was that they looked like a loading state, making people think the no further action was taken. A secondary issue was that the previous setup state caused layout shifts on selection. Some additional context from the original issue:
This PR takes a different direction, leaning into the "generic placeholder" look we recently employed for Site Logo and Featured Image:
Let me know your thoughts!
How has this been tested?
Insert a fresh navigation block, select and deselect it.
Checklist:
*.native.jsfiles for terms that need renaming or removal).