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

InputControl: Decrease large default padding if has prefix/suffix #42166

Merged
merged 8 commits into from
Jul 14, 2022
Merged

InputControl: Decrease large default padding if has prefix/suffix #42166

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
ef42dbf
InputControl: Decrease padding when prefix/suffix
mirka Jul 5, 2022
3b0e3ec
UnitControl: Remove padding override
mirka Jul 5, 2022
1a98f98
UnitControl: Clean up unused type
mirka Jul 5, 2022
8ddc126
Tweak story examples for better RTL support
mirka Jul 5, 2022
edf295b
Add changelog
mirka Jul 5, 2022
dc3e9ca
Update snapshot
mirka Jul 5, 2022
38069ad
Add comment
mirka Jul 5, 2022
8329445
Fix typo
mirka Jul 6, 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
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- `BoxControl`: Change ARIA role from `region` to `group` to avoid unwanted ARIA landmark regions ([#42094](https://github.com/WordPress/gutenberg/pull/42094)).

### Enhancements

- `InputControl`: Ensure that the padding between a `prefix`/`suffix` and the text input stays at a reasonable 8px, even in larger size variants ([#42166](https://github.com/WordPress/gutenberg/pull/42166)).

### Internal

- `Grid`: Convert to TypeScript ([#41923](https://github.com/WordPress/gutenberg/pull/41923)).
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/input-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useState, forwardRef } from '@wordpress/element';
import InputBase from './input-base';
import InputField from './input-field';
import type { InputControlProps } from './types';
import { space } from '../ui/utils/space';
import { useDraft } from './utils';

const noop = () => {};
Expand Down Expand Up @@ -85,6 +86,8 @@ export function UnforwardedInputControl(
isPressEnterToChange={ isPressEnterToChange }
onKeyDown={ onKeyDown }
onValidate={ onValidate }
paddingInlineStart={ prefix ? space( 2 ) : undefined }
paddingInlineEnd={ suffix ? space( 2 ) : undefined }
ref={ ref }
setIsFocused={ setIsFocused }
size={ size }
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/input-control/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ Default.args = {
export const WithPrefix = Template.bind( {} );
WithPrefix.args = {
...Default.args,
prefix: <span style={ { paddingLeft: 8 } }>@</span>,
prefix: <span style={ { marginInlineStart: 8 } }>@</span>,
};

export const WithSuffix = Template.bind( {} );
WithSuffix.args = {
...Default.args,
suffix: <button style={ { marginRight: 4 } }>Send</button>,
suffix: <button style={ { marginInlineEnd: 4 } }>Send</button>,
Comment on lines +43 to +49
Copy link
Member Author

Choose a reason for hiding this comment

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

Tweaked examples for better RTL.

};

export const WithSideLabel = Template.bind( {} );
Expand Down
11 changes: 11 additions & 0 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 @@ -110,6 +110,8 @@ type InputProps = {
inputSize?: Size;
isDragging?: boolean;
dragCursor?: CSSProperties[ 'cursor' ];
paddingInlineStart?: CSSProperties[ 'paddingInlineStart' ];
paddingInlineEnd?: CSSProperties[ 'paddingInlineEnd' ];
};

const disabledStyles = ( { disabled }: InputProps ) => {
Expand Down Expand Up @@ -145,6 +147,7 @@ const sizeStyles = ( {
inputSize: size,
__next36pxDefaultSize,
}: InputProps ) => {
// Paddings may be overridden by the custom paddings props.
const sizes = {
default: {
height: 36,
Expand Down Expand Up @@ -184,6 +187,13 @@ const sizeStyles = ( {
return css( style );
};

const customPaddings = ( {
paddingInlineStart,
paddingInlineEnd,
}: InputProps ) => {
return css( { paddingInlineStart, paddingInlineEnd } );
};

const dragStyles = ( { isDragging, dragCursor }: InputProps ) => {
let defaultArrowStyles: SerializedStyles | undefined;
let activeDragCursorStyles: SerializedStyles | undefined;
Expand Down Expand Up @@ -235,6 +245,7 @@ export const Input = styled.input< InputProps >`
${ disabledStyles }
${ fontSizeStyles }
${ sizeStyles }
${ customPaddings }

&::-webkit-input-placeholder {
line-height: normal;
Expand Down
13 changes: 10 additions & 3 deletions packages/components/src/input-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ export interface InputFieldProps extends BaseProps {
nextValue: string,
event?: SyntheticEvent< HTMLInputElement >
) => void;
paddingInlineStart?: CSSProperties[ 'paddingInlineStart' ];
paddingInlineEnd?: CSSProperties[ 'paddingInlineEnd' ];
setIsFocused: ( isFocused: boolean ) => void;
stateReducer?: StateReducer;
/**
Expand Down Expand Up @@ -152,12 +154,17 @@ export interface InputControlProps
* be the only prefix prop. Otherwise it tries to do a union of the two prefix properties and you end up
* with an unresolvable type.
*
* `isFocused` and `setIsFocused` are managed internally by the InputControl, but the rest of the props
* for InputField are passed through.
* `isFocused`, `setIsFocused`, `paddingInlineStart`, and `paddingInlineEnd` are managed internally by
* the InputControl, but the rest of the props for InputField are passed through.
*/
Omit<
WordPressComponentProps< InputFieldProps, 'input', false >,
'stateReducer' | 'prefix' | 'isFocused' | 'setIsFocused'
| 'stateReducer'
| 'prefix'
| 'isFocused'
| 'setIsFocused'
| 'paddingInlineStart'
| 'paddingInlineEnd'
> {
__unstableStateReducer?: InputFieldProps[ 'stateReducer' ];
}
Expand Down
10 changes: 0 additions & 10 deletions packages/components/src/unit-control/styles/unit-control-styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,13 @@ type SelectProps = {

type InputProps = {
disableUnits?: boolean;
size: SelectSize;
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated, but minor cleanup of an unused type.

};

export const Root = styled.div`
box-sizing: border-box;
position: relative;
`;

const paddingStyles = ( { disableUnits }: InputProps ) => {
if ( disableUnits ) return '';

return css`
${ rtl( { paddingRight: 8 } )() };
`;
};

Comment on lines -29 to -36
Copy link
Member Author

Choose a reason for hiding this comment

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

CSS override removed 🎉

Btw @aaronrobertshaw: When building BorderControl, do you remember if there were any blockers to rendering the BorderControlDropdown as a prefix on the UnitControl? I'm wondering if we can remove some CSS overrides in there too if this PR lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

My memory is failing me on this one, I don't recall any blockers. It might have been as simple as I didn't see the prefix prop in the UnitControl readme and didn't follow the props through to the underlying InputControl.

It also looks like we might need to tweak the UnitControl types, as attempting to pass the border control dropdown via a prefix prop on the UnitControl results in a type error that it expects prefix to be string | undefined.

I'd be happy to work on rendering the border control's dropdown as a prefix in a follow up to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds great, thanks! It's nice to see things getting less hacky as we converge on common patterns 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

I've put together a PR (#42212) that now renders the dropdown via the UnitControl prefix and attempts to clean up the styles. There could still be more to polish on that control when it comes time to add a large 40px size variant.

const arrowStyles = ( { disableUnits }: InputProps ) => {
if ( disableUnits ) return '';

Expand All @@ -58,7 +49,6 @@ export const ValueInput = styled( NumberControl )`
width: 100%;

${ arrowStyles };
${ paddingStyles };
}
}
`;
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/unit-control/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ Snapshot Diff:
class="components-unit-control-wrapper css-aa2xc3-Root e1bagdl33"
>
<div
- class="components-flex components-input-control components-number-control components-unit-control e1bagdl32 ep48uk90 em5sgkm7 css-18wzek1-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles-paddingStyles em57xhy0"
+ class="components-flex components-input-control components-number-control components-unit-control hello e1bagdl32 ep48uk90 em5sgkm7 css-18wzek1-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles-paddingStyles em57xhy0"
- class="components-flex components-input-control components-number-control components-unit-control e1bagdl32 ep48uk90 em5sgkm7 css-1njfmeu-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles em57xhy0"
+ class="components-flex components-input-control components-number-control components-unit-control hello e1bagdl32 ep48uk90 em5sgkm7 css-1njfmeu-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles em57xhy0"
data-wp-c16t="true"
data-wp-component="Flex"
>
Expand All @@ -22,7 +22,7 @@ Snapshot Diff:
>
<input
autocomplete="off"
class="components-input-control__input css-gzm6eu-Input-dragStyles-fontSizeStyles-sizeStyles em5sgkm5"
class="components-input-control__input css-1vhigq9-Input-dragStyles-fontSizeStyles-sizeStyles-customPaddings em5sgkm5"
- id="inspector-input-control-1"
+ id="inspector-input-control-2"
inputmode="numeric"
Expand Down