FontSizePicker: Add 40px size variant #42716
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
|
Size Change: +27 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Jul 27, 2022
There was a problem hiding this comment.
I think we can postpone it for the time being, because:
- I looked into adding a large size variant to RangeControl, but it is not a quick fix, and we're in the middle of a rewrite (SliderControl: Create new control using imported G2 Slider component #42315).
That sounds reasonable. Let's just keep in mind that it may take several weeks before we have a the new version of RangeControl ready, though.
Is there anything we want to change for the Reset button?
Shouldn't we apply the same size to the button as well?
| @@ -32,6 +32,8 @@ function FontSizePicker( | |||
| fontSizes = [], | |||
| disableCustomFontSizes = false, | |||
| onChange, | |||
| /** @type {'default' | '__unstable-large'} */ | |||
| size = 'default', | |||
There was a problem hiding this comment.
Adding a size prop for this is mostly to orchestrate the roll-out. For this one I think it would make sense to eventually remove the prop and make it large-only.
If we plan on removing it, should we prefix this prop with __unstable or __experimental?
Is the roll-out strategy going to be similar to other style deprecations, or are just planning on "flipping a switch"?
There was a problem hiding this comment.
These are great questions. It prompted me to think about it again, and now I'm less sure about "eventually remove the prop and make it large-only". That might not be a good idea at all, it's hard to say at the moment. So I guess we should just treat this as a normal size prop for the time being.
Yeah I expect we'll update the button component at some point to be 40px tall (rather than the current 36px). |
The thing is, I never got confirmation from Joen or Pablo that we actually do want to introduce a 40px button.
☝️ When I brought up these two instances of how a matching button size variant seems necessary, @pablohoneyhoney noted that he'd "challenge them as outdated contexts that need evolution themselves". I'm guessing that part of it is how 40px is quite large for a standard button, much less an auxiliary Reset button that doesn't need to be prominent. In any case, the Button problem seemed to be undecided at that time. It's the reason we've been holding off on making any size changes on the Button component, while every other basic component has been upsized already. So there are a couple of directions this can go:
|
Yeah I think that's a valid point. It doesn't make sense to optimise the button to cater to a UI that may soon be obsolete. Still, if we think about the components package as part of a design system I would probably expect all of these elements to share a sizing system, and the current 4px difference does feel a bit arbitrary:
If I'm not mistaken, we don't actually have any instances of this component with the reset affordance in production. So in the context of this PR, I might be inclined to just leave the reset button alone. We can then address buttons holistically, and ideate on the reset UX as separate endeavours. |
mirka
force-pushed
the
upsize-font-size-picker
branch
from
8c77c80 to
3d164f7
Compare
jameskoster
approved these changes
Aug 4, 2022
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.
Part of #41973
What?
Adds a large size variant to the FontSizePicker.
How?
Adding a
sizeprop for this is mostly to orchestrate the roll-out.For this one I think it would make sense to eventually remove the prop and make it large-only.The ToggleGroupControl, CustomSelectControl, and UnitControl inside the font size picker are all upsized. The only exception is RangeControl, which I haven't upsized for this PR. I think we can postpone it for the time being, because:
withSliderin our codebase.@jameskoster Is there anything we want to change for the Reset button?
Testing Instructions
npm run storybook:devand see the stories for FontSizePicker.