-
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
Navigation: Remove blobs that look like a loading state #37099
Conversation
Size Change: -382 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
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.
Thanks for this new approach, I think it's a good compromise between a lean placeholder and the setup menu when selected.
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. |
8125064
to
a8e0dc5
Compare
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. |
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.
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.
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
<div className="wp-block-navigation-placeholder__actions__indicator"> | |
<Icon icon={ navigation } />{ ' ' } | |
{ __( 'Navigation' ) } | |
</div> |
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 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.
There's definitely room to refactor and remove a ton of stuff.
No need in this PR. This work is great!
☝️ that used to be broken, just like it was in your screen recording.
Yep that's fixed it! 👏
Thank you for the reviews and help! |
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.js
files for terms that need renaming or removal).