Skip to content
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.

Sign up for GitHub

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

FontSizePicker: Add flag to remove bottom margin #43062

Merged
merged 8 commits into from
Aug 11, 2022
Merged

FontSizePicker: Add flag to remove bottom margin #43062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
433196d
FontSizePicker: Add no margin flag
mirka Aug 6, 2022
7bfd61b
ToggleGroupControl: Add no margin flag
mirka Aug 11, 2022
32e5780
RangeControl: Add no margin flag
mirka Aug 6, 2022
f5feb2b
FontSizePicker: Add no margin flag
mirka Aug 6, 2022
1d65cb0
Add changelog
mirka Aug 8, 2022
893bb96
Fix rerender issue
mirka Aug 10, 2022
b20cbdc
RangeControl: Fix `inline-flex`
mirka Aug 11, 2022
aa8d839
RangeControl: Fix icon wrapper height
mirka Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
- Update control labels to the new uppercase styles ([#42789](https://github.com/WordPress/gutenberg/pull/42789)).
- `UnitControl`: Update unit dropdown design for the large size variant ([#42000](https://github.com/WordPress/gutenberg/pull/42000)).
- `BaseControl`: Add `box-sizing` reset style ([#42889](https://github.com/WordPress/gutenberg/pull/42889)).
- `ToggleGroupControl`, `RangeControl`, `FontSizePicker`: Add `__nextHasNoMarginBottom` prop for opting into the new margin-free styles ([#43062](https://github.com/WordPress/gutenberg/pull/43062)).
- `BoxControl`: Export `applyValueToSides` util function. ([#42733](https://github.com/WordPress/gutenberg/pull/42733/)).

### Internal
Expand Down
234 changes: 132 additions & 102 deletions packages/components/src/font-size-picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import classNames from 'classnames';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -26,9 +31,20 @@ import {
isSimpleCssValue,
CUSTOM_FONT_SIZE,
} from './utils';
import { VStack } from '../v-stack';

// This conditional is needed to maintain the spacing before the slider in the `withSlider` case.
const MaybeVStack = ( { __nextHasNoMarginBottom, children } ) =>
! __nextHasNoMarginBottom ? (
children
) : (
<VStack spacing={ 6 } children={ children } />
);

function FontSizePicker(
{
/** Start opting into the new margin-free styles that will become the default in a future version. */
__nextHasNoMarginBottom = false,
fallbackFontSize,
fontSizes = [],
disableCustomFontSizes = false,
Expand Down Expand Up @@ -165,119 +181,133 @@ function FontSizePicker(
</FlexItem>
) }
</Flex>
<div className={ `${ baseClassName }__controls` }>
{ !! fontSizes.length &&
shouldUseSelectControl &&
! showCustomValueControl && (
<CustomSelectControl
__nextUnconstrainedWidth
className={ `${ baseClassName }__select` }
<MaybeVStack __nextHasNoMarginBottom={ __nextHasNoMarginBottom }>
<div
className={ classNames( `${ baseClassName }__controls`, {
'is-next-has-no-margin-bottom': __nextHasNoMarginBottom,
} ) }
>
{ !! fontSizes.length &&
shouldUseSelectControl &&
! showCustomValueControl && (
<CustomSelectControl
__nextUnconstrainedWidth
className={ `${ baseClassName }__select` }
label={ __( 'Font size' ) }
hideLabelFromVision
describedBy={ currentFontSizeSR }
options={ options }
value={ options.find(
( option ) =>
option.key === selectedOption.slug
) }
onChange={ ( { selectedItem } ) => {
onChange(
hasUnits
? selectedItem.size
: Number( selectedItem.size )
);
if (
selectedItem.key === CUSTOM_FONT_SIZE
) {
setShowCustomValueControl( true );
}
} }
size={ size }
/>
) }
{ ! shouldUseSelectControl && ! showCustomValueControl && (
<ToggleGroupControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
label={ __( 'Font size' ) }
hideLabelFromVision
describedBy={ currentFontSizeSR }
options={ options }
value={ options.find(
( option ) => option.key === selectedOption.slug
) }
onChange={ ( { selectedItem } ) => {
value={ value }
onChange={ ( newValue ) => {
onChange(
hasUnits
? selectedItem.size
: Number( selectedItem.size )
hasUnits ? newValue : Number( newValue )
);
if ( selectedItem.key === CUSTOM_FONT_SIZE ) {
setShowCustomValueControl( true );
}
} }
isBlock
size={ size }
/>
) }
{ ! shouldUseSelectControl && ! showCustomValueControl && (
<ToggleGroupControl
label={ __( 'Font size' ) }
hideLabelFromVision
value={ value }
onChange={ ( newValue ) => {
onChange(
hasUnits ? newValue : Number( newValue )
);
} }
isBlock
size={ size }
>
{ options.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={ `${ baseClassName }__custom-size-control` }
>
<FlexItem isBlock>
<UnitControl
label={ __( 'Custom' ) }
labelPosition="top"
hideLabelFromVision
value={ value }
onChange={ ( nextSize ) => {
if (
0 === parseFloat( nextSize ) ||
! nextSize
) {
onChange( undefined );
} else {
onChange(
hasUnits
? nextSize
: parseInt( nextSize, 10 )
);
}
} }
size={ size }
units={ hasUnits ? units : [] }
{ options.map( ( option ) => (
<ToggleGroupControlOption
key={ option.key }
value={ option.value }
label={ option.label }
aria-label={ option.name }
showTooltip={ true }
/>
</FlexItem>
{ withReset && (
) ) }
</ToggleGroupControl>
) }
{ ! withSlider &&
! disableCustomFontSizes &&
showCustomValueControl && (
<Flex
justify="space-between"
className={ `${ baseClassName }__custom-size-control` }
>
<FlexItem isBlock>
<Button
className="components-color-palette__clear"
disabled={ value === undefined }
onClick={ () => {
onChange( undefined );
<UnitControl
label={ __( 'Custom' ) }
labelPosition="top"
hideLabelFromVision
value={ value }
onChange={ ( nextSize ) => {
if (
0 === parseFloat( nextSize ) ||
! nextSize
) {
onChange( undefined );
} else {
onChange(
hasUnits
? nextSize
: parseInt(
nextSize,
10
)
);
}
} }
isSmall
variant="secondary"
>
{ __( 'Reset' ) }
</Button>
size={ size }
units={ hasUnits ? units : [] }
/>
</FlexItem>
) }
</Flex>
) }
</div>
{ withSlider && (
<RangeControl
className={ `${ baseClassName }__custom-input` }
label={ __( 'Custom Size' ) }
value={ ( isPixelValue && noUnitsValue ) || '' }
initialPosition={ fallbackFontSize }
onChange={ ( newValue ) => {
onChange( hasUnits ? newValue + 'px' : newValue );
} }
min={ 12 }
max={ 100 }
/>
) }
{ withReset && (
<FlexItem isBlock>
<Button
className="components-color-palette__clear"
disabled={ value === undefined }
onClick={ () => {
onChange( undefined );
} }
isSmall
variant="secondary"
>
{ __( 'Reset' ) }
</Button>
</FlexItem>
) }
</Flex>
) }
</div>
{ withSlider && (
<RangeControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
className={ `${ baseClassName }__custom-input` }
label={ __( 'Custom Size' ) }
value={ ( isPixelValue && noUnitsValue ) || '' }
initialPosition={ fallbackFontSize }
onChange={ ( newValue ) => {
onChange( hasUnits ? newValue + 'px' : newValue );
} }
min={ 12 }
max={ 100 }
/>
) }
</MaybeVStack>
</fieldset>
);
}
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/font-size-picker/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export default {
title: 'Components/FontSizePicker',
component: FontSizePicker,
argTypes: {
__nextHasNoMarginBottom: { control: { type: 'boolean' } },
initialValue: { table: { disable: true } }, // hide prop because it's not actually part of FontSizePicker
fallbackFontSize: {
description:
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/font-size-picker/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
max-width: $sidebar-width - ( 2 * $grid-unit-20 );
align-items: center;
margin-top: $grid-unit-10;
margin-bottom: $grid-unit-30;

&:not(.is-next-has-no-margin-bottom) {
margin-bottom: $grid-unit-30;
}

.components-unit-control-wrapper {
.components-input-control__label {
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/range-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function UnforwardedRangeControl< IconProps = unknown >(
forwardedRef: ForwardedRef< HTMLInputElement >
) {
const {
__nextHasNoMarginBottom = false,
afterIcon,
allowReset = false,
beforeIcon,
Expand Down Expand Up @@ -212,6 +213,7 @@ function UnforwardedRangeControl< IconProps = unknown >(

return (
<BaseControl
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ciampo There is an ever so slight space here that I can only resolve by setting a line-height: 0 on .components-base-control__field, and I don't know what is happening 😂 Let me know if you have any ideas.

Small space after the input field

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the typical "white space" issue that happens around inline elements — there are different ways to fix, including setting font-size: 0 or line-height: 0 on .components-base-control__field.

But in my opinion those are just "patchy" fixes — a better way to handle it is to avoid having inline elements, which cause the white space in the first place.

The easiest fix would be to change RangeControl's Root element from display: inline-flex to display: flex.

In case there was a reason for that Root element to be inline-flex and we don't want to touch it, another alternative is to set display: flex; flex-direction: column on .components-base-control__field (also in this scenario, we'd need to check that this change doesn't introduce any regression).

Bonus round: for both proposed solutions, we could use HStack or VStack instead!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had so much trouble finding the culprit, thank you 😂

The easiest fix would be to change RangeControl's Root element from display: inline-flex to display: flex.

I went with this fix — it looks safe from what I could gather.

__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
className={ classes }
label={ label }
hideLabelFromVision={ hideLabelFromVision }
Expand All @@ -225,6 +227,7 @@ function UnforwardedRangeControl< IconProps = unknown >(
</BeforeIconWrapper>
) }
<Wrapper
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
className={ wrapperClasses }
color={ colorProp }
marks={ !! marks }
Expand Down
12 changes: 9 additions & 3 deletions packages/components/src/range-control/styles/range-control-styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const thumbSize = 12;
export const Root = styled.div`
-webkit-tap-highlight-color: transparent;
align-items: flex-start;
display: inline-flex;
display: flex;
justify-content: flex-start;
padding: 0;
position: relative;
Expand All @@ -40,8 +40,12 @@ export const Root = styled.div`
const wrapperColor = ( { color = COLORS.ui.borderFocus }: WrapperProps ) =>
css( { color } );

const wrapperMargin = ( { marks }: WrapperProps ) =>
css( { marginBottom: marks ? 16 : undefined } );
const wrapperMargin = ( { marks, __nextHasNoMarginBottom }: WrapperProps ) => {
if ( ! __nextHasNoMarginBottom ) {
return css( { marginBottom: marks ? 16 : undefined } );
}
return '';
};

export const Wrapper = styled.div< WrapperProps >`
color: ${ COLORS.blue.medium.focus };
Expand All @@ -56,12 +60,14 @@ export const Wrapper = styled.div< WrapperProps >`
`;

export const BeforeIconWrapper = styled.span`
display: flex; // ensures the height isn't affected by line-height
margin-top: ${ railHeight }px;

${ rtl( { marginRight: 6 } ) }
`;

export const AfterIconWrapper = styled.span`
display: flex; // ensures the height isn't affected by line-height
margin-top: ${ railHeight }px;

${ rtl( { marginLeft: 6 } ) }
Expand Down
7 changes: 5 additions & 2 deletions packages/components/src/range-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export type ControlledRangeValue = number | '' | null;

export type RangeControlProps< IconProps = unknown > = Pick<
BaseControlProps,
'hideLabelFromVision' | 'help'
'hideLabelFromVision' | 'help' | '__nextHasNoMarginBottom'
> &
MarksProps & {
/**
Expand Down Expand Up @@ -243,7 +243,10 @@ export type InputRangeProps = {
value?: number | '';
};

export type WrapperProps = {
export type WrapperProps = Pick<
BaseControlProps,
'__nextHasNoMarginBottom'
> & {
color?: CSSProperties[ 'color' ];
marks?: RangeMarks;
};
Expand Down
Loading