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

Components: Decrease standard padding to 12px #64708

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Components: Decrease standard padding to 12px #64708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1eaa86c
Components: Decrease standard padding to 12px
mirka Aug 22, 2024
0b6ed7a
Fixup
mirka Aug 22, 2024
76f4075
Update snapshots
mirka Aug 22, 2024
debf0ff
Add changelog
mirka Aug 22, 2024
45ca2aa
Fixup
mirka Aug 22, 2024
de3ad38
Merge branch 'trunk' into less-input-padding
mirka Aug 22, 2024
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
16 changes: 6 additions & 10 deletions packages/components/src/color-picker/hex-input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ import { __ } from '@wordpress/i18n';
*/
import { InputControl } from '../input-control';
import { Text } from '../text';
import { Spacer } from '../spacer';
import { space } from '../utils/space';
import { COLORS } from '../utils/colors-values';
import type { StateReducer } from '../input-control/reducer/state';
import type { HexInputProps } from './types';
import InputControlPrefixWrapper from '../input-control/input-prefix-wrapper';

export const HexInput = ( { color, onChange, enableAlpha }: HexInputProps ) => {
const handleChange = ( nextValue: string | undefined ) => {
Expand Down Expand Up @@ -48,14 +47,11 @@ export const HexInput = ( { color, onChange, enableAlpha }: HexInputProps ) => {
return (
<InputControl
prefix={
<Spacer
as={ Text }
marginLeft={ space( 4 ) }
color={ COLORS.theme.accent }
lineHeight={ 1 }
>
#
</Spacer>
<InputControlPrefixWrapper>
<Text color={ COLORS.theme.accent } lineHeight={ 1 }>
#
</Text>
</InputControlPrefixWrapper>
Comment on lines +50 to +54
Copy link
Member Author

Choose a reason for hiding this comment

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

ColorPicker hex input

}
value={ color.toHex().slice( 1 ).toUpperCase() }
onChange={ handleChange }
Expand Down
16 changes: 6 additions & 10 deletions packages/components/src/color-picker/input-with-slider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
*/
import { HStack } from '../h-stack';
import { Text } from '../text';
import { Spacer } from '../spacer';
import { space } from '../utils/space';
import { RangeControl, NumberControlWrapper } from './styles';
import { COLORS } from '../utils/colors-values';
import type { InputWithSliderProps } from './types';
import InputControlPrefixWrapper from '../input-control/input-prefix-wrapper';

export const InputWithSlider = ( {
min,
Expand Down Expand Up @@ -39,14 +38,11 @@ export const InputWithSlider = ( {
value={ value }
onChange={ onNumberControlChange }
prefix={
<Spacer
as={ Text }
paddingLeft={ space( 4 ) }
color={ COLORS.theme.accent }
lineHeight={ 1 }
>
{ abbreviation }
</Spacer>
<InputControlPrefixWrapper>
<Text color={ COLORS.theme.accent } lineHeight={ 1 }>
{ abbreviation }
</Text>
</InputControlPrefixWrapper>
Comment on lines +41 to +45
Copy link
Member Author

Choose a reason for hiding this comment

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

ColorPicker RGB input

}
spinControls="none"
size="__unstable-large"
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/custom-select-control-v2/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ const ANIMATION_PARAMS = {
};

const INLINE_PADDING = {
compact: 8, // space(2)
small: 8, // space(2)
default: 16, // space(4)
compact: CONFIG.controlPaddingXSmall,
small: CONFIG.controlPaddingXSmall,
default: CONFIG.controlPaddingX,
Comment on lines +24 to +26
Copy link
Member Author

Choose a reason for hiding this comment

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

CustomSelectControlV2

};

const getSelectSize = (
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/input-control/input-base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ function InputBase(
} );
const prefixSuffixContextValue = useMemo( () => {
return {
InputControlPrefixWrapper: { paddingLeft },
InputControlSuffixWrapper: { paddingRight },
InputControlPrefixWrapper: { paddingLeft: `${ paddingLeft }px` },
InputControlSuffixWrapper: { paddingRight: `${ paddingRight }px` },
};
}, [ paddingLeft, paddingRight ] );

Expand Down
17 changes: 8 additions & 9 deletions packages/components/src/input-control/styles/input-control-styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { Flex, FlexItem } from '../../flex';
import { Text } from '../../text';
import { baseLabelTypography, COLORS, CONFIG, rtl } from '../../utils';
import type { LabelPosition, Size } from '../types';
import { space } from '../../utils/space';

type ContainerProps = {
disabled?: boolean;
Expand Down Expand Up @@ -188,29 +187,29 @@ export const getSizeConfig = ( {
height: 40,
lineHeight: 1,
minHeight: 40,
paddingLeft: space( 4 ),
paddingRight: space( 4 ),
paddingLeft: CONFIG.controlPaddingX,
paddingRight: CONFIG.controlPaddingX,
Comment on lines +190 to +191
Copy link
Member Author

Choose a reason for hiding this comment

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

InputControl

InputControl

NumberControl with custom steppers

NumberControl

UnitControl

UnitControl

},
small: {
height: 24,
lineHeight: 1,
minHeight: 24,
paddingLeft: space( 2 ),
paddingRight: space( 2 ),
paddingLeft: CONFIG.controlPaddingXSmall,
paddingRight: CONFIG.controlPaddingXSmall,
},
compact: {
height: 32,
lineHeight: 1,
minHeight: 32,
paddingLeft: space( 2 ),
paddingRight: space( 2 ),
paddingLeft: CONFIG.controlPaddingXSmall,
paddingRight: CONFIG.controlPaddingXSmall,
},
'__unstable-large': {
height: 40,
lineHeight: 1,
minHeight: 40,
paddingLeft: space( 4 ),
paddingRight: space( 4 ),
paddingLeft: CONFIG.controlPaddingX,
paddingRight: CONFIG.controlPaddingX,
},
};

Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/item-group/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ const paddingYLarge = `calc((${ CONFIG.controlHeightLarge } - ${ baseFontHeight

export const itemSizes = {
small: css`
padding: ${ paddingYSmall } ${ CONFIG.controlPaddingXSmall };
padding: ${ paddingYSmall } ${ CONFIG.controlPaddingXSmall }px;
`,
medium: css`
padding: ${ paddingY } ${ CONFIG.controlPaddingX };
padding: ${ paddingY } ${ CONFIG.controlPaddingX }px;
Copy link
Member Author

Choose a reason for hiding this comment

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

ItemGroup, medium size

`,
large: css`
padding: ${ paddingYLarge } ${ CONFIG.controlPaddingXLarge };
padding: ${ paddingYLarge } ${ CONFIG.controlPaddingXLarge }px;
`,
};
10 changes: 5 additions & 5 deletions packages/components/src/select-control/styles/select-control-styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import styled from '@emotion/styled';
/**
* Internal dependencies
*/
import { COLORS, rtl } from '../../utils';
import { COLORS, rtl, CONFIG } from '../../utils';
import { space } from '../../utils/space';
import type { SelectControlProps } from '../types';
import InputControlSuffixWrapper from '../../input-control/input-suffix-wrapper';
Expand Down Expand Up @@ -108,10 +108,10 @@ const sizePaddings = ( {
selectSize = 'default',
}: SelectProps ) => {
const padding = {
default: 16,
small: 8,
compact: 8,
'__unstable-large': 16,
default: CONFIG.controlPaddingX,
Copy link
Member Author

Choose a reason for hiding this comment

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

SelectControl

small: CONFIG.controlPaddingXSmall,
compact: CONFIG.controlPaddingXSmall,
'__unstable-large': CONFIG.controlPaddingX,
};

if ( ! __next40pxDefaultSize ) {
Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/text-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@

// Subtract 1px to account for the border, which isn't included on the element
// on newer components like InputControl, SelectControl, etc.
padding-left: $grid-unit-20;
padding-right: $grid-unit-20;
// These values should be shared with the `controlPaddingX` in ./utils/config-values.js
padding-left: $grid-unit-15;
padding-right: $grid-unit-15;
Comment on lines +26 to +28
Copy link
Member Author

Choose a reason for hiding this comment

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

TextControl

}
}
10 changes: 6 additions & 4 deletions packages/components/src/utils/config-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@ import { space } from './space';
import { COLORS } from './colors-values';

const CONTROL_HEIGHT = '36px';
const CONTROL_PADDING_X = '12px';
Copy link
Member Author

Choose a reason for hiding this comment

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

We should soon make a decision about how we want to define our foundational variables — number, string, or function (space())? I am kind of leaning toward numbers because it's more versatile. For example we can make most calculations at compile time rather than rely on all the calc()s at runtime.

Copy link
Contributor

@ciampo ciampo Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely.

A few considerations that come to mind (more of a brain dump):

  • Is this something that should come directly from theming?
  • FWIW, I don't think that calc() has a big perf impact, browsers should be quite fast at calculating it
  • Using calc and CSS variables would enable a different style of coding — we would be able to change the value of a variable, instead of re-writing a new formula
    -I agree that dealing with numbers is more practical. We could still have an underlying grid system, but we should consider exposing variables/tokens expressing numbers


const CONTROL_PROPS = {
controlSurfaceColor: COLORS.white,
controlTextActiveColor: COLORS.theme.accent,
controlPaddingX: CONTROL_PADDING_X,
controlPaddingXLarge: `calc(${ CONTROL_PADDING_X } * 1.3334)`,
controlPaddingXSmall: `calc(${ CONTROL_PADDING_X } / 1.3334)`,

// These values should be shared with TextControl.
controlPaddingX: 12,
controlPaddingXSmall: 8,
controlPaddingXLarge: 12 * 1.3334, // TODO: Deprecate
Copy link
Contributor

Choose a reason for hiding this comment

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

If ItemGroup is the only consumer of this variable, I guess it makes sense to just delete it from the common config and inline the value in ItemGroup? Also, should we consider aligning ItemGroup to the rest of the components and have a 12px padding too?

Copy link
Member Author

Choose a reason for hiding this comment

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

ItemGroup already does have a 12px padding in the default size, interestingly. We'll probably deprecate the controlPaddingXLarge variable and the large size of ItemGroup at the same time.


controlBackgroundColor: COLORS.white,
controlBoxShadow: 'transparent',
controlBoxShadowFocus: `0 0 0 0.5px ${ COLORS.theme.accent }`,
Expand Down
Loading