-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Use ToolsPanel in Global Styles → Typography #44180
Conversation
Size Change: +6.91 kB (+1%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
What do you think of this rough proof of concept @jasmussen? It would solve the problems we have with there being no way to reset Decoration, Letter case, and Font size in Global Styles. The only thing I don't like is that it means there are now two headers. Ideally we would consolidate the two headers circled in yellow and find somewhere else to put the controls that are in the ellipsis circled in red so that there aren't so many ellipsis buttons. So something like this: |
I think this is an excellent solution, and would agree with the consolidation into a single header, if possible. But even with the subheading (perhaps without the separating border? 🤔 ) it would be an improvement over the reset buttons. The main challenge with the reset buttons is that there's no system in how to place them, their labels change width and wrap depending on languages, and they get undue prominence considering they are secondary to the main action. Anything to move forward from them would be a win, IMO! |
Thanks for the gut check @jasmussen! @ciampo @mirka: Any thoughts on whether we should / how we could make <ToolsPanel>
<HStack>
<Button icon={ arrowLeft } label="Back" />
<Heading>Headings</Heading>
<ToolsPanelMenu resetAll={ resetAll } />
</HStack>
<ToolsPanelItem>...</ToolsPanelItem>
<ToolsPanelItem>...</ToolsPanelItem>
</ToolsPanel> |
Would it make sense for this panel to have all the possible settings showing? We hide/show selectively for blocks because there's too many settings to show at once (spacing, colors, typography...) but in Global Styles it's a more limited amount, and displaying all at the same time would reduce the amount of clicking, which is already high as it is to get to the actual element we want to customize. |
Yeah! I think so. We only care about the reset functionality that ToolsPanel provides, not the hidden-by-default functionality that ToolsPanel provides. In this very rough PR I have it so that all the controls are visible by default. |
I don't have enough familiarity with this component to think of a good approach off hand, but we should definitely figure something out if this is a pattern we're going to use. The approach you're suggesting sounds reasonable, and I think it's worth trying to see if we can do it cleanly. (I'm betting that we can, because the current ToolsPanel code seems pretty well equipped to deal with refactors like this!) |
We could look into doing that, and making changes to If we go for the modular approach, we could see if we always want a |
I think the whole header. We could allow a JSX element to be given as the title but it could get tricky to make sure e.g. the back button and ellipsis menu are aligned. |
I spent some time exploring how to combine the two headers in #44284. There's something there but I think it's more effort than it's worth, at least right now. How about we keep the two headers but pull the heading level selector and font preview out of the typography panel? This creates space between the two headers, communicates the information hierarchy a lot more clearly, and makes the panel look more like the one in the block inspector.
What do you think @jasmussen? |
I merged Kapture.2022-09-21.at.16.53.11.mp4 |
To keep things simple, it does seem fine to keep the separator above the typography panel. But just to clarify some of the conversation about where to put the ellipsis, I was thinking that we could be as simple as just removing the border: The video you shared looks good, except I'd keep the preview above the h1-h6. Possibly even above the description, a la #42919 (comment). Mainly a detail. Is that helpful? |
I did try this but having the border feels more right to me. What I like about the border is that it makes the Typography UI here look identical to the Typography UI in the block inspector. I expect that users who have learned what the ellipsis does there will instantly know what it does here too.
I moved the preview to be above the heading level selector. Here's how this PR looks now: I could move it further up to be above the description. Here's how that would look: I don't have a strong preference. The former is more consistent with other panels which always have the description below the title. The latter is more consistent with the explorations in #42919 (comment).
Always, Joen! I respect your judgement on these things more than mine. |
This is looking great to me so far. I noticed that the font family control doesn't appear to have the same checks as other typography properties, i.e., it appears in the tools panel options list even where there is no font family control. At least in Empty Theme, for which no font families are defined in theme.json 😄 2022-09-26.14.16.56.mp4 |
Can confirm that the font family tools panel item only appears where a theme has font family presets, e.g., 2022Empty themeThank you! Resetting heading typography styles works for individual headings, e.g.., clicking "Reset all" for H1 will reset H1 typography styles only and not other headings, but I wasn't sure what "Reset all" on the "All" tab should do. My assumption is that it should result all headings (H1-H6), but for me it doesn't: 2022-09-29.10.25.37.mp4 |
Hm yeah. I'd say the behaviour you're seeing is correct, and I even think the information hierarchy (the fact that the heading level selector appears above the Typography panel) helps to clarify what will happen, just that it's confusing that we're using the word "all" to refer to both "all heading levels" (in the heading selector) "all typography options" (in the dropdown). How about we relabel "Reset all" in the dropdown to say "Reset heading settings", "Reset H1 settings", etc.? |
An alternative way way we could clarify that is maybe by relabelling the "All" option in the heading level selector to be something like "Default" or "Base" since what you're actually editing there is the base heading styles which can be overridden by an individual heading level's style. For example the font family selected in "H1" takes precedence over the font family selected in "All". |
Ah I get it now, thanks. So the "All" tab behaves similarly to the other tabs – "H1" etc – resets are mutually exclusive in that the reset functions only take effect for changes made in that tab. In that light, things are working as expected then 😄
Good suggestion. I like. Look, it could be just me that was confused. Happy to be schooled here, but that would be clearer for my 🧠 |
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.
Tools panel functionality works, and typography styles are applied as expected, thanks @noisysocks
Would be good to confirm with @jasmussen whether the comments in #44180 (comment) are indeed a problem or it's just down to me circuiting out 🙃
Sounds like an easy follow up though
Yeah that's right. Part of the problem is that we're using a
The problem though is Default doesn't fit inside the button 😅
Yeah let's merge this as-is and defer to @jasmussen on whether this is an actual problem or something we've invented. I'd like to get this in as I think it's a big improvement over what's in trunk. I'll no doubt continue to iterate on this panel within the context of #42919. |
Thanks for working on this. I think the best path forward is to try this out in the plugin and get a feel for it, I'm sure that if it becomes a problem in practice, it will resurface. Nice work! |
*/ | ||
import { useStyle } from './hooks'; | ||
|
||
export default function TypographyPreview( { name, element, headingLevel } ) { |
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 noticed that to show the list of elements in the "typography-screen", we also "preview" the typography styles but in a smaller context (the buttons). It might be worth trying to use this component there as well.
What?
Updates Global Styles → Typography to use
ToolsPanel
. This allows users to reset styles to the theme default using the familiar ellipsis menu flow.Why?
Being able to reset the value is extremely useful, especially when editing styles for blocks (e.g. Global Styles → Blocks → Heading → Typography).
It makes the Global Styles interface consistent with the Block Settings interface.
It means that we don't have to invent new (and inconsistent) UIs for resetting values. For example, right now there is no way to reset the font size when using the T-shirt size selector.
It means we could have more advanced controls hidden by default so as to not overwhelm the user. (Not doing this yet.)
It lets us hide the ugly Reset button in the font size picker when setting a custom font size.
How?
We already do this for Dimensions so I basically copied that approach.
Testing Instructions
Screenshots or screencast
Kapture.2022-09-21.at.16.53.11.mp4