Change header height from 60px to 64px #60729
Conversation
|
Size Change: +8 B (0%) Total Size: 1.87 MB
ℹ️ View Unchanged
|
| 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.
| margin-top: 60px; | ||
| margin-top: $header-height; |
There was a problem hiding this comment.
Same as this comment
386b7e6 to
cd1d26f
Compare
| 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.
| 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).
There was a problem hiding this comment.
Maybe another good spot for adding a comment?
|
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. |
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 branch56px |
@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. 56px64px |
|
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 |
9c223a0 to
95d9afe
Compare
| @@ -178,7 +178,7 @@ | |||
| @include break-small() { | |||
| height: $grid-unit-50; | |||
| position: relative; | |||
| top: -5px; // Should be -4px, but that causes scrolling when focus lands on the movers, in a 60px header. | |||
| top: #{$grid-unit-05 * -1}; | |||
There was a problem hiding this comment.
Should we retain the comment, so that folks know the reason for the -1px ?
There was a problem hiding this comment.
I've made it even clearer by 1bf3037 what the intent of these styles is, including the top property.
There was a problem hiding this comment.
Can use perhaps $border-width instead of 1px, that might imply the comment.
There was a problem hiding this comment.
$border-width instead of 1px
Here 1px is not used. #{$grid-unit-05 * -1} becomes -4px. Am I missing something?
| height: $header-height + $grid-unit-15; | ||
| height: $header-height + $grid-unit-10; |
There was a problem hiding this comment.
Maybe another good spot for adding a comment?
|
Flaky tests detected in f3e576d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11566701735
|
f3e576d to
c9aaff7
Compare
|
I'd love to see this land. How can we help? |
|
Given 6.9 isn't due until December now feels like a good moment in the release cycle to try this adjustment. Seemingly the missing piece is whether to go with 64px or 56px. Despite the 4px increase vs trunk I lean towards 64 for the reasons outlined in #60729 (comment), but also because I think it will enable some coherent alignment outside of the Editor that enables this button to be anchored in place throughout the UI: |
|
If a 64px height is acceptable, this PR is ready to ship. I've double-checked to see if the issue reported in the comment still occurs on Windows with a 56px height. The reason is that the appearance of the default scrollbar in the Chrome browser has recently changed.
This issue does not seem to occur anymore. This may be related to the recent slight change to the scrollbar UI in the Chrome browser on Windows.
Unfortunately, this issue is still reproducible: |
|
Let's try this new header height. If there are many negative comments, we can easily revert it back. |
* Change header height from 60px to 64px * Improve comments for vertical block mover * Add comment for Modal style Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
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.