-
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
Visual improvements to the block navigator #22796
Conversation
padding-top: $grid-unit-30 !important; | ||
padding-bottom: $grid-unit-30 !important; |
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.
let's get rid of these !important
before merging
.block-editor-block-navigation-leaf { | ||
transition: background 0.1s ease-out; | ||
@include reduce-motion("transition"); | ||
|
||
> :first-child { | ||
padding-left: $grid-unit-20; | ||
} | ||
> :last-child { | ||
padding-right: $grid-unit-20; | ||
} | ||
|
||
&:hover { | ||
background: $light-gray-300; | ||
} | ||
|
||
.block-editor-block-navigation-block-contents { | ||
padding-top: $grid-unit-15; | ||
padding-bottom: $grid-unit-15; | ||
} | ||
} | ||
|
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.
These changes should live in components/block-navigation/style.scss
if they should also apply to the post editor block navigator.
@@ -74,7 +81,7 @@ $tree-item-height: 36px; | |||
|
|||
.block-editor-block-mover-button { | |||
width: $button-size; | |||
height: $button-size; | |||
height: 16px; |
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.
Movers PR will supersede these customizations
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.
Do you mean this one? #22673
Size Change: +794 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Before: After: In short, this is a massive improvement. Well done. A few small things:
One thing we need to fix is the row-height. It's 36px which doesn't afford big enough tap targets: We should make it 48px tall, so that it can afford up and down mover buttons that are 24x24 both: |
@jasmussen wait, the "after" gif you posted looks different from what I see on my end:
Sure! Looking into these now |
Ah no, it's the same, I just tested this in the block navigator that is invoked in the Navigation block, whereas you're showing it in context of the edit-menus screen! Which is good, becasue it should be the same in both. |
@jasmussen here's how it looks like in the "global" block navigator in the post editor: It seems a bit off, maybe the background shouldn't react to hover there? Or only the button should light up? |
That's possible, but I'd rather move it to another PR as it would involve grabbing the ID of the current home and posts pages from the API.
It is 48px now, although the movers are visually further apart from each other: This could be addressed by moving the arrows closer to the button's edge, but I'm not too sure about this approach: Maybe we should just keep them more spread apart. I'm curious about your opinion. |
This one is ready for a re-review |
@jasmussen I fixed the margin issues aside of this one:
Unfortunately I am not able to reproduce it. Would you mind taking it for a spin if it still shows up on your end? |
The inserter is all @noisysocks in #22590, I just rebased :-) If it looks good, would you mind approving? |
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.
Great work here @adamziel. 🎉
I noticed a few minor style problems when the branch is rebased against master
that might need to be tweaked :
- The settings menu icon button is misaligned, might just need some padding adjustment (I think there was a separate PR that adjusted it's alignment)
- Getting the appender lined up with the block icons would be great
- There's a slight overlap between the focus ring and the descending line.
And then I noticed there was a comment about removing some !important
s from the css. Hopefully those should all be relatively easy things though.
…ck navigator (this naming is so confusing!)
69a1fd5
to
a5de001
Compare
Awesome! I made a few tweaks to fix these minor issues you mentioned Dan. Should any other tweaks be necessary, let's address then in a follow-up PR. |
Description
This PR is an initial take on the design changes proposed in #22497
What's missing