Editor Interface: Remove small header from global styles/plugin sidebar #64474
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
t-hamano
commented
Aug 13, 2024
Comment on lines
-87
to
-93
| ### smallScreenTitle | ||
|
|
||
| In small screens, the complementary area may take up the entire screen. | ||
| `smallScreenTitle` allows displaying a title above the complementary area, so the user is more aware of what the area refers to. | ||
|
|
||
| - Type: `String` | ||
| - Required: No |
There was a problem hiding this comment.
I'm wondering if we should simply remove this prop, or show a deprecation message.
There was a problem hiding this comment.
I like simply removing it but wouldn’t say I know best.
|
Size Change: -386 B (-0.02%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
t-hamano
force-pushed
the
interface/remove-small-header
branch
from
b461c36
to
ac9b9a2
Compare
|
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. |
|
Thank you, this is all working as advertised for me. |
|
Thanks so much for working on this, this is an important one! 🙏 |
stokesman
approved these changes
Aug 26, 2024
Comment on lines
-87
to
-93
| ### smallScreenTitle | ||
|
|
||
| In small screens, the complementary area may take up the entire screen. | ||
| `smallScreenTitle` allows displaying a title above the complementary area, so the user is more aware of what the area refers to. | ||
|
|
||
| - Type: `String` | ||
| - Required: No |
There was a problem hiding this comment.
I like simply removing it but wouldn’t say I know best.
|
@stokesman Thanks for the feedback! After thinking about it, I realized that developers who use the What do you think? |
t-hamano
changed the title
Perhaps. The caution is good and may be the side to err on. Still, I don’t think I can see utility in the Another consideration that may factor in is that a plugin consuming the interface package has to bundle it and so cannot be directly broken by new releases of the package. Maybe that permits moving forward with the removed prop. If so, a dev note seems a good idea along with a changelog entry. Totally separate but I wonder what’s up with the excessive line-height as seen in your screenshot (at "Add your own CSS…of your site…"):
I’m curious as it looks okay to me on Chrome/MacOS. |
I didn't know this! The If that's the case, adding a changelog and writing a dev note would be enough.
The problem turned out to be caused by the link having a top margin. The issue wasn't apparent on the desktop view, but the PR rexposes additional CSS for mobile, so it's causing an issue:
I solved it by adding the
|
t-hamano
added
the
Needs Dev Note
I’d forgotten it until I tested the MailPoet plugin on this branch then noticed the changes don’t affect it and had to remember why.
It‘s true that a WordPress update will have the latest Nice that you fixed the little issue making the line-height look off. Makes sense. This LGTM again. You may wish to have another member give it a sanity check though. |
t-hamano
force-pushed
the
interface/remove-small-header
branch
from
aec761b
to
d010b1a
Compare
|
Flaky tests detected in d010b1a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11456987547
|
|
Tests well for me. Screenshot before:
After:
|
|
Closed #62067 manually. GitHub only recognizes the first issue after the unique keyword. If you want to link multiple issues to a single PR, use the keyword for each one. Example: |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ar (WordPress#64474) Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
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 #64095, #62067
What?
This PR removes the small screen header from theComplementaryAreaHeadercomponent. This means that thesmallScreenTitleprop on theComplementaryAreacomponent will be removed.This PR removes the small screen header from the global styles sidebar and plugin sidebar.
Why?
As mentioned in #62067, it seems that a header for small screens was necessary up until WP6.3:
However, now that the default header has exactly the same functionality, it seems that a header for small screens is no longer necessary.
How?
This PR also exposes some header buttons (revisions, more menu) that were intentionally hidden on smaller screens.
This makes the following features available on smaller screens:
I initially thought about removing the
smallScreenTitleprop from theComplementaryAreacomponent, but it seems that there are plugins that use theComplementaryAreacomponent directly to build their own sidebars.smallScreenTitleprop defined in the MailPoet pluginI decided to keep this prop because these plugins may still want to provide the title for mobile:Update: This component is not exposed in the global wp object. Developers must explicitly bundle the interface package. This means that unless developers update their interface package, it will not be affected by this PR. Therefore, we decided to remove the
smallScreenTitleprop.Testing Instructions
Verify that the following screens are displayed correctly on a small screen and that the newly exposed features work.
Screenshots or screencast
Block Inspector
Global Styles
PluginsSidebar
Run the following code in your browser console:
Details
Global Styles Revisions
Additional CSS