Fix the headings hierarchy in the styles sidebar #43848
Merged
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
afercia
added
[Type] Bug
|
Size Change: +142 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
fix/styles-sidebar-headings-hierarchy
branch
from
07b6aff to
a43c5cc
Compare
afercia
force-pushed
the
fix/styles-sidebar-headings-hierarchy
branch
from
a43c5cc to
668d710
Compare
|
I have fixed the two small merge conflicts. I have browsed through all the screens / panels and the levels look correct to me. I found an exception that can be solved in a separate PR, and that is a missing H1 when the style book is opened:
|
carolinan
approved these changes
Jan 19, 2023
|
Flaky tests detected in ec355e2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4084473743
|
|
@afercia can you please rebase this PR to resolve the conflicts? Once it gets rebased, we can continue with the review 👍 |
afercia
force-pushed
the
fix/styles-sidebar-headings-hierarchy
branch
from
4032d18 to
ec355e2
Compare
|
Rebased on top of latest trunk. |
aristath
approved these changes
Feb 6, 2023
ciampo
reviewed
Feb 6, 2023
There was a problem hiding this comment.
Thank you for working on this, @afercia — this is definitely a great improvement!
For future work involving the @wordpress/components package, it would be great if you could ping @mirka and/or myself so that we can help and advise.
Especially when working on API design, and ever more so for low-level packages like components/data/etc.., we need to think carefully about the choices we make given the strict back-compat policy.
Finally, when making changes in the components package, we usually add an item to the package's CHANGELOG.
Furthermore, tomorrow marks the feature freeze deadline for the WordPress 6.2 release — @afercia would be able to follow-up and apply any agreed changes to the components before tomorrow ?
Edit: I went ahead and opened #47788
| @@ -313,6 +313,7 @@ export default function PaletteEdit( { | |||
| colors = EMPTY_ARRAY, | |||
| onChange, | |||
| paletteLabel, | |||
| paletteLabelLevel = 2, | |||
There was a problem hiding this comment.
I don't think that this prop name conveys its meaning, since "level" only makes sense in the context of a "heading".
I suggest that we rename this prop to a more clear name, maybe paletteLabelHeadingLevel?
| /** | ||
| * The heading level of the panel's header. | ||
| */ | ||
| headingLevel: HeadingSize; |
There was a problem hiding this comment.
I believe this prop should be marked as optional as assigned a default value, both for ToolsPanel and ToolsPanelHeader. Any reason why it was marked as required instead?
Have you considered adding yourself as code owners? It is great to know who to ping, but not so easy to remember when you work on so many different parts of the project. |
|
Sorry for late response, pretty busy days and I missed to reply. Tanks for your feedback @ciampo and for the polishing in #47788. Much appreciated. All your points make totally sense. Just a few considerations. I'd second @carolinan's comment, as it is important to make contributors live easier.
Is this policy documented anywhere? Not arguing, but I think for the greater good of any open source software project it's very, very important to make all contributors well aware of the best practices they should follow. I'm all for a wise back-compat policy but it should be documented and shared. Any contributor deserves to be placed in the best possible conditions for a valuable contribution. Also, that would help to make the best use of their time and of the reviewers' time.
I'm not arguing on this point as well 🙏 Is this documented anywhere, though? Maybe I'm missing something because of my low vision but I can't see anything related to changelog entries in the components package CONTRIBUTING.md or README.md. I just see generic recommendations to 'ensure documentation'. |
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 #42496
What?
Changes the heading levels in the styles sidebar so that the headings hierarchy is correct.
Why?
The heading hierarchy must reflect the logical structure of the content.
Previously, the first heading in the styles sidebar was a H5, which skipped many levels and didn't make sense.
Also, sections of content that belong to a main 'topic' need a heading level to indicate they're 'sub-headings'.
How?
Headingcomponents.Headingcomponent, adds new props where necessary.Regarding
ToolsPanel, I'm not that familiar with TypeScript types, please do let me know if the way I imported and used types is correct.Testing Instructions
The best way to test this PR is by installing the HeadingsMap browser extension (available also for Firefox).
Screenshots or screencast
A few screenshots:
Typography panel:
Colors panel:
Colors > Palette panel:
Layout panel: