-
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
Global Styles: Upsize typography panel components #42718
Conversation
Size Change: +123 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
33780b2
to
43c710a
Compare
43c710a
to
cb1b99a
Compare
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.
Thanks for the continued efforts wrangling component heights and panel layouts @mirka 👍
The upsized controls in the Typography panel test well for me. I didn't spot any issues comparing this PR to trunk.
One small question, should the Buttons typography panel's font size control reset button also be upsized?
Inconsistencies in horizontal spacing are pre-existing
Did this mean to say vertical spacing?
We decided to leave it for the time being, but we do need to deal with it at some point. Looking around at the Global Styles UIs, I see a bit of inconsistency since a few controls show their own Reset buttons (ColorPalette, FontSizePicker) even though the rest of the controls simply rely on the "Reset to defaults" in the actions menu. Maybe the right move here is to remove the Reset buttons. (cc @jameskoster @jasmussen)
Yes 🙈 |
cc @jasmussen and @jameskoster |
Radios switching contextual control views is fine, like in your yes/no example. The radio is actually selecting between a mutually exclusive set of values (yes or no) that is to be persisted. If I choose No, toggle some of the contextual controls, and then switch back to Yes before hitting submit, the contextual values I selected for No are ignored. The problem is when the radio is only being used as a view switcher, not as a value selector.
So even setting HTML semantics aside, it actually causes confusion for sighted users if the radio/tabpanel UIs aren't visually differentiated within a design system. Hope that makes sense! |
This is very helpful, thank you for the patience, yes.
I definitely understand this for pure radio groups. But for the segmented control (speaking in terms of design moreso than engine), I commonly see that applied across mobile apps (mainly iOS) as a tab switcher. I'm unaware of any expectation that this design has to behave like a radiogroup. Just to compare, visually the iOS Segmented Control appears identical to the Tab view. Could we do something similar? |
Yes, I think so! As long as we're mindful of the behavioral difference that they each carry, and we give it sufficient affordance to differentiate between them. In Pablo's mockups, the segmented control was at a thinner, 32px height when used as a tabpanel switcher (#42008). We can pair this 32px segmented control with tabpanel semantics and we're good to go, as I noted in that PR while you were away:
We opted to stick with the existing TabPanel design at that time, but we can change that. |
Thank you! Something we can look at when we get time. |
Part of #41973
What?
Updates the Typography panel in Global Styles to render 40px size controls.
Why?
The Typography panel in Global Styles is a nice isolated section of the UI to start upsizing components to the 40px height.
How?
Set the
size
prop on all the controls used in this section.Testing Instructions
npm run dev
Inconsistencies in horizontal spacing are pre-existing, and will be addressed separately in conjunction with #39358.
Screenshots or screencast
Global Styles ▸ Typography ▸ Headings:
(☝️ Aside: Here is another place where the segmented control is begging for
tabpanel
semantics 😅)