FontSizePicker: Replace SCSS with Emotion + components #44483
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
noisysocks
added
[Type] Code Quality
noisysocks
commented
Sep 27, 2022
| `; | ||
|
|
||
| // 280px is the sidebar width. | ||
| // TODO: Remove this, @wordpress/components shouldn't care what a "sidebar" is. |
There was a problem hiding this comment.
Didn't want to change the UI in this PR so will do this later.
|
Size Change: -293 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
noisysocks
changed the title
There was a problem hiding this comment.
Thanks for all the work in tackling the FontSizePicker refactor to TypeScript @noisysocks 🙇
I am likely missing some context regarding the goals for the FontSizePicker styling so take all the questions below with a grain of salt 🧂
Is there a reason we are favouring styled components here over dynamic classes?
Would it be better if the styles were responsible for applying a bottom margin or not rather than conditionally wrapping components?
Click for a quick and dirty example
diff --git a/packages/components/src/font-size-picker/index.tsx b/packages/components/src/font-size-picker/index.tsx
index 6172a1a695..c3673339ad 100644
--- a/packages/components/src/font-size-picker/index.tsx
+++ b/packages/components/src/font-size-picker/index.tsx
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import type { ReactNode, ForwardedRef } from 'react';
+import type { ForwardedRef } from 'react';
/**
* WordPress dependencies
@@ -20,6 +20,7 @@ import { Flex, FlexItem } from '../flex';
import { default as UnitControl, useCustomUnits } from '../unit-control';
import CustomSelectControl from '../custom-select-control';
import { VisuallyHidden } from '../visually-hidden';
+import { useCx } from '../utils/hooks/use-cx';
import {
ToggleGroupControl,
ToggleGroupControlOption,
@@ -31,6 +32,7 @@ import {
isSimpleCssValue,
CUSTOM_FONT_SIZE,
} from './utils';
+import { VStack } from '../v-stack';
import { HStack } from '../h-stack';
import type {
FontSizePickerProps,
@@ -41,24 +43,11 @@ import {
Container,
HeaderHint,
HeaderLabel,
- Controls,
+ controls,
ResetButton,
} from './styles';
import { Spacer } from '../spacer';
-const MaybeMarginBottom = ( {
- __nextHasNoMarginBottom,
- children,
-}: {
- __nextHasNoMarginBottom: boolean;
- children: ReactNode;
-} ) =>
- __nextHasNoMarginBottom ? (
- <>{ children }</>
- ) : (
- <Spacer marginBottom={ 6 }>{ children }</Spacer>
- );
-
const UnforwardedFontSizePicker = (
props: FontSizePickerProps,
ref: ForwardedRef< any >
@@ -83,6 +72,14 @@ const UnforwardedFontSizePicker = (
} );
}
+ const cx = useCx();
+ const controlsClassName = useMemo( () => {
+ return cx(
+ controls( __nextHasNoMarginBottom ),
+ 'components-font-size-picker__controls'
+ );
+ }, [ cx, __nextHasNoMarginBottom ] );
+
const hasUnits = [ typeof value, typeof fontSizes?.[ 0 ]?.size ].includes(
'string'
);
@@ -199,149 +196,132 @@ const UnforwardedFontSizePicker = (
) }
</HStack>
</Spacer>
- <MaybeMarginBottom
- __nextHasNoMarginBottom={ __nextHasNoMarginBottom }
- >
- <Controls
- className="components-font-size-picker__controls"
- spacing={ 6 }
- >
- { !! fontSizes.length &&
- shouldUseSelectControl &&
- ! showCustomValueControl && (
- <CustomSelectControl
- __nextUnconstrainedWidth
- className="components-font-size-picker__select"
- label={ __( 'Font size' ) }
- hideLabelFromVision
- describedBy={ currentFontSizeSR }
- options={ options as FontSizeSelectOption[] }
- value={ (
- options as FontSizeSelectOption[]
- ).find(
- ( option ) =>
- option.key === selectedOption.slug
- ) }
- onChange={ ( {
- selectedItem,
- }: {
- selectedItem: FontSizeSelectOption;
- } ) => {
- onChange?.(
- hasUnits
- ? selectedItem.size
- : Number( selectedItem.size )
- );
- if (
- selectedItem.key === CUSTOM_FONT_SIZE
- ) {
- setShowCustomValueControl( true );
- }
- } }
- size={ size }
- />
- ) }
- { ! shouldUseSelectControl && ! showCustomValueControl && (
- <ToggleGroupControl
- __nextHasNoMarginBottom
+ <VStack className={ controlsClassName } spacing={ 6 }>
+ { !! fontSizes.length &&
+ shouldUseSelectControl &&
+ ! showCustomValueControl && (
+ <CustomSelectControl
+ __nextUnconstrainedWidth
+ className="components-font-size-picker__select"
label={ __( 'Font size' ) }
hideLabelFromVision
- value={ value }
- onChange={ ( newValue ) => {
+ describedBy={ currentFontSizeSR }
+ options={ options as FontSizeSelectOption[] }
+ value={ ( options as FontSizeSelectOption[] ).find(
+ ( option ) => option.key === selectedOption.slug
+ ) }
+ onChange={ ( {
+ selectedItem,
+ }: {
+ selectedItem: FontSizeSelectOption;
+ } ) => {
onChange?.(
- hasUnits ? newValue : Number( newValue )
+ hasUnits
+ ? selectedItem.size
+ : Number( selectedItem.size )
);
+ if ( selectedItem.key === CUSTOM_FONT_SIZE ) {
+ setShowCustomValueControl( true );
+ }
} }
- isBlock
size={ size }
- >
- { ( options as FontSizeToggleGroupOption[] ).map(
- ( option ) => (
- <ToggleGroupControlOption
- key={ option.key }
- value={ option.value }
- label={ option.label }
- aria-label={ option.name }
- showTooltip={ true }
- />
- )
- ) }
- </ToggleGroupControl>
+ />
) }
- { ! withSlider &&
- ! disableCustomFontSizes &&
- showCustomValueControl && (
- <Flex
- justify="space-between"
- className="components-font-size-picker__custom-size-control"
- >
+ { ! shouldUseSelectControl && ! showCustomValueControl && (
+ <ToggleGroupControl
+ __nextHasNoMarginBottom
+ label={ __( 'Font size' ) }
+ hideLabelFromVision
+ value={ value }
+ onChange={ ( newValue ) => {
+ onChange?.(
+ hasUnits ? newValue : Number( newValue )
+ );
+ } }
+ isBlock
+ size={ size }
+ >
+ { ( options as FontSizeToggleGroupOption[] ).map(
+ ( option ) => (
+ <ToggleGroupControlOption
+ key={ option.key }
+ value={ option.value }
+ label={ option.label }
+ aria-label={ option.name }
+ showTooltip={ true }
+ />
+ )
+ ) }
+ </ToggleGroupControl>
+ ) }
+ { ! withSlider &&
+ ! disableCustomFontSizes &&
+ showCustomValueControl && (
+ <Flex
+ justify="space-between"
+ className="components-font-size-picker__custom-size-control"
+ >
+ <FlexItem isBlock>
+ <UnitControl
+ label={ __( 'Custom' ) }
+ labelPosition="top"
+ hideLabelFromVision
+ value={ value }
+ onChange={ ( nextSize ) => {
+ if (
+ ! nextSize ||
+ 0 === parseFloat( nextSize )
+ ) {
+ onChange?.( undefined );
+ } else {
+ onChange?.(
+ hasUnits
+ ? nextSize
+ : parseInt( nextSize, 10 )
+ );
+ }
+ } }
+ size={ size }
+ units={ hasUnits ? units : [] }
+ />
+ </FlexItem>
+ { withReset && (
<FlexItem isBlock>
- <UnitControl
- label={ __( 'Custom' ) }
- labelPosition="top"
- hideLabelFromVision
- value={ value }
- onChange={ ( nextSize ) => {
- if (
- ! nextSize ||
- 0 === parseFloat( nextSize )
- ) {
- onChange?.( undefined );
- } else {
- onChange?.(
- hasUnits
- ? nextSize
- : parseInt(
- nextSize,
- 10
- )
- );
- }
+ <ResetButton
+ className="components-color-palette__clear"
+ disabled={ value === undefined }
+ onClick={ () => {
+ onChange?.( undefined );
} }
+ isSmall
+ variant="secondary"
size={ size }
- units={ hasUnits ? units : [] }
- />
+ >
+ { __( 'Reset' ) }
+ </ResetButton>
</FlexItem>
- { withReset && (
- <FlexItem isBlock>
- <ResetButton
- className="components-color-palette__clear"
- disabled={ value === undefined }
- onClick={ () => {
- onChange?.( undefined );
- } }
- isSmall
- variant="secondary"
- size={ size }
- >
- { __( 'Reset' ) }
- </ResetButton>
- </FlexItem>
- ) }
- </Flex>
- ) }
- { withSlider && (
- <RangeControl
- __nextHasNoMarginBottom
- className="components-font-size-picker__custom-input"
- label={ __( 'Custom Size' ) }
- value={
- isPixelValue && noUnitsValue
- ? Number( noUnitsValue )
- : undefined
- }
- initialPosition={ fallbackFontSize }
- onChange={ ( newValue ) => {
- onChange?.(
- hasUnits ? newValue + 'px' : newValue
- );
- } }
- min={ 12 }
- max={ 100 }
- />
+ ) }
+ </Flex>
) }
- </Controls>
- </MaybeMarginBottom>
+ { withSlider && (
+ <RangeControl
+ __nextHasNoMarginBottom
+ className="components-font-size-picker__custom-input"
+ label={ __( 'Custom Size' ) }
+ value={
+ isPixelValue && noUnitsValue
+ ? Number( noUnitsValue )
+ : undefined
+ }
+ initialPosition={ fallbackFontSize }
+ onChange={ ( newValue ) => {
+ onChange?.( hasUnits ? newValue + 'px' : newValue );
+ } }
+ min={ 12 }
+ max={ 100 }
+ />
+ ) }
+ </VStack>
</Container>
);
};
diff --git a/packages/components/src/font-size-picker/styles.ts b/packages/components/src/font-size-picker/styles.ts
index 5354d78844..a80e8c396a 100644
--- a/packages/components/src/font-size-picker/styles.ts
+++ b/packages/components/src/font-size-picker/styles.ts
@@ -2,6 +2,7 @@
* External dependencies
*/
import styled from '@emotion/styled';
+import { css } from '@emotion/react';
/**
* Internal dependencies
@@ -10,7 +11,6 @@ import BaseControl from '../base-control';
import Button from '../button';
import { space } from '../ui/utils/space';
import { COLORS } from '../utils';
-import { VStack } from '../v-stack';
import type { FontSizePickerProps } from './types';
export const Container = styled.fieldset`
@@ -30,8 +30,9 @@ export const HeaderHint = styled.span`
// 280px is the sidebar width.
// TODO: Remove this, @wordpress/components shouldn't care what a "sidebar" is.
-export const Controls = styled( VStack )`
+export const controls = ( hasNoMarginBottom?: boolean ) => css`
max-width: calc( 280px - ${ space( 4 ) } * 2 );
+ margin-bottom: ${ hasNoMarginBottom ? 0 : space( 6 ) };
`;
export const ResetButton = styled( Button )< {
I'm sure @ciampo or @mirka would have a more informed opinion on this.
EDIT from @ciampo: adding lena's correct GitHub handle
ciampo
reviewed
Sep 27, 2022
There was a problem hiding this comment.
As also written in an inline comment, the fact that we don't pass __nextHasNoMarginBottom anymore down to child components, and instead we rely on layout specific to this component means that, if we ever change the margin bottom on ToggleGroupControl or RangeControl, those new margins won't be reflected in FontSizePicker. I would, therefore, personally prefer an approach for this PR that doesn't refactor the DOM structure / layout strategy of the component (or that at least forwards __nextHasNoMarginBottom down to children components)
Would it be better if the styles were responsible for applying a bottom margin or not rather than conditionally wrapping components?
I believe this was Lena's approach, but I'm personally not opposed to using classnames as an alternative.
I also spotted 2 differences compared to trunk:
- When
size='__unstable-large', the Reset button ontrunkstill looks shorter, while on this PR it matches the input's height. I may remember incorrectly, but I think this was on purpose (as we haven't yet introduced newsizepresets toButton)
| trunk | this PR |
|---|---|
 |
 |
- The custom font size slider should be full width, while it's not in this PR
| trunk | this PR |
|---|---|
 |
 |
They both achieve the same result and in fact the
I wanted to use
When But yeah ok I'll change this back so that the margin comes from
I noticed that the Reset button has a custom height so thought that I may as well make this custom height respect
Good catch, will look into that 👍 |
|
OK try that @ciampo. I couldn't confirm that that second issue is fixed using the snapshot tests. For some reason they pass locally for me even though I know there should be an error (the Reset button height). |
noisysocks
force-pushed
the
update/font-size-picker-to-use-emotion
branch
from
35d7b32 to
331f1f7
Compare
ciampo
approved these changes
Sep 28, 2022
There was a problem hiding this comment.
I wanted to use Spacer, VStack, etc. as much as possible because it's nice when we can implement new components using existing components and not write any CSS at all.
I definitely see your point and you're right in saying that we should aim for using our layout components instead of writing custom CSS — in this case, though, I'd rather keep the "layout logic" as on trunk to keep this PR focus on the Emotion refactor, and also because most of the "legacy layout" should go away soon anyway, as we remove __nextHasNoMarginBottom and force all components not to have an outer margin bottom out of the box.
I noticed that the Reset button has a custom height so thought that I may as well make this custom height respect __unstable-large so that it looks less weird. Happy to not do this if you prefer. We won't use the Reset button after #44180 so it's of no real consequence either way.
I also think that it looks weird, but my preference would be to keep the height of Reset button the same as on trunk (i.e. always 30px regardless of the size prop). We can always tackle it separately in the future and keep this PR focused on the refactor to Emotion (cc @mirka )
Finally, while testing changes from this PR I realized that HeaderLabel currently doesn't support RTL languages as expected (the hint should be rendered to the left of "Size") — although this is also an issue on trunk and can definitely be improved in a follow-up PR, if you prefer so.
In short: we can merge once the Reset button height is adjusted to how it currently is on trunk (and once the Black Parade is over)
|
Sounds good! I made the Reset button 30px again. |
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.
What?
Updates
FontSizePickerto use Emotion + G2 components instead of SCSS.Why?
One step closer to all of
@wordpress/componentsusing this styling system. Plus I'm about to make some changes to this component so nice to get the house in order first.How?
I put The Black Parade on and got to work.
Testing Instructions
npm run storybook:devFontSizePickerstory should look and work the same as it does in wordpress.github.io/gutenberg.