Change header height from 60px to 64px #60729
Open
+8
−8
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
t-hamano
added
[Type] Enhancement
|
Size Change: -22 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
t-hamano
commented
Apr 14, 2024
| margin-top: 60px; | ||
| margin-top: $header-height; |
There was a problem hiding this comment.
This is the top margin of the content that will be displayed when a fatal error occurs in the block editor. This value was originally hardcoded, but it should be expected to match the height of the header.
t-hamano
commented
Apr 14, 2024
| margin-top: 60px; | ||
| margin-top: $header-height; |
There was a problem hiding this comment.
Same as this comment
t-hamano
force-pushed
the
change-header-height-64px
branch
from
386b7e6
to
cd1d26f
Compare
t-hamano
commented
Apr 14, 2024
| top: $header-height + $grid-unit-15; | ||
| top: $header-height + $grid-unit-10; |
There was a problem hiding this comment.
Required in the Pattern Inserter modal to make the sidebar exactly adjacent to the modal header.
t-hamano
commented
Apr 14, 2024
| height: $header-height + $grid-unit-15; | ||
| height: $header-height + $grid-unit-10; |
There was a problem hiding this comment.
Required to maintain the current modal header height which is 72px (60px + 12px = 64px + 8px).
t-hamano
requested review from
ajitbohra,
draganescu,
talldan,
adamziel,
kevin940726 and
ellatrix
as code owners
|
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. |
jasmussen
approved these changes
Apr 15, 2024
There was a problem hiding this comment.
Wonderful PR. Clearly articulated, well tested, well commented. Nice work. It also seems to work well for me across both site and posts editors in my testing.
It comes with a nice side effect of the padding above and below the inserter button now being 16px, matching the left padding, and causing a uniform bit of whitespace.
|
Love it. 56px would also work (producing consistent 12px space around buttons), and would result in slightly less horizontal space being used (more breathing room for document title). No strong feelings either way: This branch
56px
|
@jasmussen what're your thoughts here? I lean towards 56px. The smaller the height, the less dramatic the animation into the editor canvas is from the Site Editor. It feels a bit chunky. |
|
I'm not particular about either 56px or 64px, but there is one problem with 56px. If the top toolbar is enabled, focusing on the block mover will slightly shift the entire block toolbar up. 25ccc17c9438d7d28bf66e99e1f857b8.mp4This is a problem that was also attempted to be solved in #57444, which introduced a custom scrollbar, and probably only occurs on Windows OS. We finally solved this problem by shifting the mover button slightly up when the top toolbar is enabled. If we want the header to be 56px high, we'll need to move this mover button a little higher (maybe 2px?). Below is the style that was added to fix the focus shift. gutenberg/packages/edit-post/src/components/header/style.scss Lines 102 to 108 in 9d77296 |
I like 64px, I think 56 can work too. Screens aren't getting smaller, though, and lately I've found myself appreciating when there's room to breathe. I wonder; we're between major releases, would it be feasible to try 64 for a bit in the plugin, see how that feels after having used it in practice, and then regroup? |
|
I remembered that there are plugins that inject their own buttons into the header. Default buttons have already normalized at 32px, but most of the buttons injected by plugins are 36px. In the future, buttons with a new default size of 40px may be injected. Considering this, 64px might be better so it doesn't look cramped. The screenshots below are the header after activating two plugins All in One SEO and Yoast SEO. 56px
64px
|
|
Thank you everyone for the reviews. What do you think about shipping this PR as is? That means the header height will be 64px. For now, I think it's better to leave some leeway. |
|
I'm ok with giving it a run for some time while we're mid-cycle, but I am still hesitant on making it bigger without a tangible benefit. |
|
I resolved the conflict.
Regarding this issue, if the header height was 64px, I was able to align the mover button perfectly without causing any shifts. Before:
After:
The video below shows that no focus shift occurs. 4f480c8ae26fa66bf542f7b3d98605c6.mp4 |
|
I'm game to try this. But I'd love @richtabor on board as well. |
|
Should we merge this for the 6.7 cycle? What do you think @richtabor ? |
|
I am still a bit hesitant to take up more vertical space than necessary. What's the primary objective for increasing the height of the header? |
Here, we have two options, 56px and 64px. In the case of 56px, the mover button moves slightly upwards when focused. This problem probably occurs only on Windows OS and Chrome. We have tried various approaches, but there is no solution to this problem so far. Also, I discovered that in the case of 56px, the bottom side of the focus outline seems to be slightly cut off.
Check out the video below to see what happens when the mover button gets focus. 56px56pc.mp464px64px.mp4 |
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.
Fixes #58293
Part of #46734
What?
This PR changes the block editor header height from 60px to 64px.
Why?
As part of the unification of UI component sizes proposed in #46734. This comment also suggests changing the header height to 64px.
How?
Changed the value of the
$header-heightCSS variable from 60px to 64px. At the same time, I made adjustments to the areas affected by this change.Testing Instructions
We'll probably need to do extensive testing to make sure this change doesn't cause a 4px shift. I haven't found any issues in my testing, but if you have any concerns please let me know.
Note
As reported in #55568, the
npm run devcommand does not detect changes to thebase-stylespackage. So you need to runnpm run devagain after checking out the branch.