[Components] Update FontSize control #35395
Conversation
ntsekouras
added
[Package] Components
ntsekouras
requested review from
mtias,
mirka,
ciampo,
jasmussen,
jorgefilipecosta,
pablohoneyhoney and
aaronrobertshaw
|
Size Change: +598 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Hi, Nik I just wanted to note that we want this component to be backward compatible with unitless values. See #33679. We have unit tests in place, so the regression should be easy to catch. |
|
Thanks for starting work on this one. It's a big one, it's a difficult one, and it's so nice to get something on the blank canvas. Here's a GIF of what I saw in a quick testing:
I'll need to come back to this one with deeper thoughts, but a few quick superficial observations:
This is just one small ingredient of a large effort, nice work, and I think many of the pieces will start falling into place once we get this one down. |
|
Thanks for the first tests @jasmussen and great remarks!
Yes, it makes sense to fix/augment existing components that are going to be used. I made the draft to pause a bit and check some related PRs like the
I know about that and haven't decided yet how to handle the options (default + custom) as it will also be coupled with |
Thanks for the ping. It's great to see progress on the new typography components. 👍 I did have some tweaks to the current If you'd prefer for me to put those changes on ice until this PR evolves, let me know. Even as it is now, the version of Other than the reset button, the only other tweak for the Applying these changes alongside #33744 appears to work well and I'd expect it to evolve without many conflicts. Screen.Recording.2021-10-07.at.5.23.22.pm.mp4I did encounter one small issue while testing which was in a couple of themes the select options included
|
Hey @jasmussen , that definitely makes sense (although in a separate issue/PR). We should be adding any changes that are necessary to improve the typography panel in tracking issue #34345, so that we can take it from there and coordinate around these tasks. With respect to what @aaronrobertshaw said, that sounds like a good plan to me! |
I'll look into this, thanks! Regarding the reset I'm going to try to maybe have a first iteration with the reset button and the current |
ntsekouras
force-pushed
the
try/font-size-control-changes
branch
2 times, most recently
from
04ce574 to
57c323e
Compare
|
I've updated the control to switch to I think it's ready for another round of testing with different themes etc 😄 |
| list-style-type: none; | ||
| padding: $grid-unit-10; | ||
| cursor: default; | ||
| line-height: $icon-size + $grid-unit-05; | ||
|
|
||
|
|
||
| &.has-hint { | ||
| grid-template-columns: auto auto 30px; |
There was a problem hiding this comment.
Can we handle this better? 🤔 --cc @jasmussen
There was a problem hiding this comment.
Have you considered using the Grid component instead of using CSS styles?
There was a problem hiding this comment.
Can you expand on what I should look for here? Happy to help if I can.
There was a problem hiding this comment.
What I mean was to refactor this component to use internally the Grid component — instead of using custom CSS rules for the grid layout.
There was a problem hiding this comment.
I wanted to make the minimum changes in CustomSelectControl and I'm not sure there are some better rules I should use - or even maybe flex? Maybe I'll consider splitting these changes to a separate PR as Marco suggested..
|
Took this one for a quick spin, and my goodness, even though parts are missing, this is just a big step forward: In the mockups, the fields use a new all-caps subheading style. I imagine this is something we can follow up with separately, but for now we should at least remove the thin font weight from this one: Instead of |
There was a problem hiding this comment.
Thank you for starting work on this PR, @ntsekouras ! As others have also said, it definitely represents a big improvement over the current UI.
I've been taking a more deeper look at the code and left a few comments. We should also rebase this PR to include a few changes to the FontSizePicker component that got merged in the meantime.
Regarding the proposal of rewriting these components to TypeScript and use Emotion, I'm more than happy to help (either directly, or indirectly with advice and reviews).
Finally, I just wanted to cross-post the fact that #35451 may contain some overlapping changes (cc'ing the PR author, @aaronrobertshaw )
packages/components/src/toggle-group-control/toggle-group-control-option.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-backdrop.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| const baseClassName = 'components-font-size-picker'; |
There was a problem hiding this comment.
I know that this classname existed before this PR, but it got me thinking — it'd be also great if we could ride the wave of changes to FontSizePicker to also switch its styling from Sass to Emotion, and convert it to TypeScipt (of course in a separate PR)
There was a problem hiding this comment.
Sounds good, but let's do this separately..
There was a problem hiding this comment.
Yep, that sounds like a good follow-up
| { item.hint && ( | ||
| <span className="components-custom-select-control__item-hint"> | ||
| { item.hint } | ||
| </span> | ||
| ) } |
There was a problem hiding this comment.
I think it would be better to split changes to CustomSelectControl in a separate PR, where the relevant documentation is also updated and a CHANGELOG entry is added.
The current PR will keep focusing on the FontSizePicker, making changes more contained and easier to find in the future.
Separately from this, I also wonder if we should rewrite the CustomSelectControl component to use Emotion for styling and rewrite it to TypeScript. These improvements would ease the integration of these components with the Typography panel and give more confidence to our refactors (via TYpeScript's static linting)
ntsekouras
force-pushed
the
try/font-size-control-changes
branch
from
abce09e to
8107ffe
Compare
Yes. I saw that but this affects all control labels and we should be consistent. In a separate PR we should update all controls to uppercase - css probably(?) as third party controls can be there..
Good call 😄 - I updated the styles. |
I believe a similar issue was raised in #35400 (comment). We've got #35464 open to potentially update the |
ntsekouras
requested review from
ajitbohra,
chrisvanpatten,
nerrad and
ntwb
as code owners
|
Taking this one for a spin again, it feels pretty solid to me: There are a number of small things we need to fix, but I don't think they need to happen as part of this PR. For example, the custom font size icon is offset from the right: This padding is also what makes the toggle wider, and scales down the icon inside:
The right way to fix that is to revisit the CSS that adds this padding in the first place, which appears to be this: However in the name of shipping this, we could add the following in the mean time? Happy to push if need be. This would fix it:
|
|
Also wanted to note that the items inside the segmented control follow the order of font sizes defined in theme.json. I had them in large to small descending order, and when correcting them to be small to large, this is reflected in the component:
👍 👍 |
|
I can also confirm that if I add additional values, more than 5, they show up in the custom dropdown: Edit: Actually, let's replace that font-weight: 200; with a $gray-700 color. And the dropdown looks reasonably good, and can be improved separately. From a design point of view, I'd be happy to greenlight this one if we can fix the custom font size icon, as outlined here, and I'd also personaly be okay with bespoke CSS to fix it, and then follow up on that separately. |
|
Since there's already a CSS file that customizes this panel, I went ahead and pushed a small fix to the icon size, padding, position and width, and changed the font weight in the dropdown. Looks good to me now: |
|
Tested other instances of the CustomSelectControl, to verify they hadn't regressed. It's used for font appearance, and for date format. Easy ways to test is to insert a Post Date block and check the format in the inspector: Or the tagline block and check the font appearance: All looks solid to me. |
There was a problem hiding this comment.
This feels solid to me, and a big step forward. From the design side, there are a few small things to follow up on, but most of them fall in the category of general components work (40px input fields, consume components without having to write bespoke CSS), and not something that should block this PR.
I tested it with a few font size presets, and with many. In both cases the experience works well for me, using both mouse and keyboard. I also verified that the enhancements made to the CustomSelectControl have not caused trouble with any of the existing usages of the component. In that light, this one feels safe.
I'd be happy to land this one as a first iteration and then follow up if anything appears.
|
Hey @ntsekouras 👋 I just tested these changes on mobile and it's working well, thanks for the ping! 👍 |
|
Thank you all for your help here!!! 🙏 |
|
🔥 |
|
It's good to see progress happening so fast! One thing that I'd like to note, though, is that we included changes to Introducing a lot of fast changes, especially in a package like the components package, can become counter-productive very quickly. |
|
Updated |
You're right about that and I was thinking maybe it's better to make this prop experimental for now and revisit when the typescript/css-in-js is handled for this component ? 🤔 |
This PR is part of Typography tool update: #34345 and is focuses on the
FontSizePickercontrol.The purpose of this PR is to try to see what is missing and how we will move forward to match the current design and also maybe revisit them to take into account our actual needs. We should make this iteratively and decide on these iterations. Also don't mind the actual code too much, as is mostly exploratory and will need refactoring.
Screenshots
Current design
And also from this: #34345 (comment)
These first explorations have already raised some issues and some of them are things from changes I've not committed here.
For a first iteration we could probably skip the dual control, but we'll see 😄 :
Notes
Control Label
We need to decide what should be shown next to the
Sizelabel in all cases (custom, from existing option, default, etc..)Custom Select component
Does changes in this components sound good like I've done in this PR? It would need better css handling though if we keep these..
Also the design is a bit different with the mock up as there is no space above the showed options.
Range Control
Currently the control supports only unitless values. We need to make this work with
units. We would also need 'position handling' support as it currently shows theslideron the left.Testing instructions
paragraphand check thefont sizecontrol.