-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Most used tags: Try fixing label #44859
Conversation
@@ -1,3 +1,4 @@ | |||
.interface-interface-skeleton__actions, |
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.
So this is obviously a big change, given it's SCSS. But at the moment, it targets the entire inspector, why shouldn't it target also the pre-publish inspector? Would love a sanity check on this one. Thank you!
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.
Just speaking from a general CSS architecture standpoint, this kind of thing isn't encouraged because it breaks component encapsulation. Styles for InterfaceSkeleton
should not live in a different component's folder.
// Subheading style. | |
h3 { | |
font-size: 11px; | |
text-transform: uppercase; | |
font-weight: 500; | |
color: $gray-900; | |
margin-bottom: 1.5em; | |
} |
And assuming that the reason you want to do this is to get these uppercase styles ☝️, this kind of styling (targeting all h3
s) also isn't generally tenable in a componentized app.
So my quick fix suggestion for this specific issue would be to change this:
gutenberg/packages/editor/src/components/post-taxonomies/most-used-terms.js
Lines 50 to 52 in db3b647
<h3 className="editor-post-taxonomies__flat-term-most-used-label"> | |
{ label } | |
</h3> |
to:
<BaseControl.VisualLabel as="h3" className="editor-post-taxonomies__flat-term-most-used-label">
{ label }
</BaseControl.VisualLabel>
so we can get the label styles cleanly. Let me know if you need further assistance, and I can push a commit to this branch if you would like.
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.
Please feel free to push to this branch!
Thank you.
Size Change: +59 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
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.
The fixes look great. Thank you, Joen!
I can't think of why complementary area styles shouldn't target the per-publish inspector. The panel designs are mostly identical.
Let's see if others have any objections to this change.
Thanks. I'll let this one sit for others to chime in on, since it isn't particularly urgent. |
This reverts commit 3f4f335.
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.
I pushed up my suggestions. Good to go on my end if it still does what you intended to do initially 👍
What?
Most used tags is a thin weight:
I think it was designed for a previous style of labels that weren't small and uppercase. This PR just removes the font weight:
The prepublish interface was also missing the style entirely:
I made a biggish change to the code that styles the default inspector, to also encompass the pre-publish sidebar. I'll comment in the code and ask for an extra look:
Testing Instructions
Have at least 4 different labels, and 3 of them used on a bunch of posts. Then observe the Tags sidebar, and the pre publish tag suggestion dialog.