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

Deprecate bottom margin on BaseControl-based components #64408

Merged
merged 5 commits into from
Aug 13, 2024
Merged

Deprecate bottom margin on BaseControl-based components #64408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dae9b5
BaseControl: Deprecate bottom margin
mirka Aug 9, 2024
2b2fb30
Propagate to components
mirka Aug 9, 2024
0b5efce
Missed spots
mirka Aug 9, 2024
e553654
Add changelog
mirka Aug 12, 2024
807b1a3
List all affected components in changelog
mirka Aug 13, 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
17 changes: 17 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,23 @@

## Unreleased

### Deprecations

- Deprecate bottom margin on the following `BaseControl`-based components. Set the `__nextHasNoMarginBottom` prop to true to start opting into the new styles, which will become the default in a future version ([#64408](https://github.com/WordPress/gutenberg/pull/64408)).
- `BaseControl`
- `CheckboxControl`
- `ComboboxControl`
- `DimensionControl`
- `FocalPointPicker`
- `RangeControl`
- `SearchControl`
- `SelectControl`
- `TextControl`
- `TextareaControl`
- `ToggleControl`
- `ToggleGroupControl`
- `TreeSelect`

### New Features

- `Composite`: add stable version of the component ([#63564](https://github.com/WordPress/gutenberg/pull/63564)).
Expand Down
13 changes: 13 additions & 0 deletions packages/components/src/base-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { ForwardedRef } from 'react';
/**
* WordPress dependencies
*/
import deprecated from '@wordpress/deprecated';
import { forwardRef } from '@wordpress/element';

/**
Expand All @@ -31,6 +32,7 @@ const UnconnectedBaseControl = (
) => {
const {
__nextHasNoMarginBottom = false,
__associatedWPComponentName = 'BaseControl',
Copy link
Member

Choose a reason for hiding this comment

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

Should we disallow usage of this prop outside the @wordpress/components package? Right now, anyone can modify it, which shouldn't be allowed outside the package, IMHO. It can be a simple ESLint rule.

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 think it's fine. If anyone cares enough to use this prop it's probably for a good reason. I'm imagining an extender that has a component library using BaseControl as a wrapper, and they want to show better deprecation messages to their own consumers.

In the Gutenberg repo, any direct usages of BaseControl moving forward shouldn't throw the deprecation warning in the first place (we lint for this). So I think it's a non-issue for now.

id,
label,
hideLabelFromVision = false,
Expand All @@ -39,6 +41,17 @@ const UnconnectedBaseControl = (
children,
} = useContextSystem( props, 'BaseControl' );

if ( ! __nextHasNoMarginBottom ) {
deprecated(
`Bottom margin styles for wp.components.${ __associatedWPComponentName }`,
{
since: '6.7',
version: '7.0',
hint: 'Set the `__nextHasNoMarginBottom` prop to true to start opting into the new styles, which will become the default in a future version.',
}
);
}

return (
<Wrapper className={ className }>
<StyledField
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/base-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ export type BaseControlProps = {
* @default false
*/
__nextHasNoMarginBottom?: boolean;
/**
* Temporary private prop for showing better deprecation messages,
* e.g. `Some feature from wp.components.${ __associatedWPControl } is deprecated`.
*
* @ignore
*/
__associatedWPComponentName?: string;
/**
* The HTML `id` of the control element (passed in as a child to `BaseControl`) to which labels and help text are being generated.
* This is necessary to accessibly associate the label with that element.
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/checkbox-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export function CheckboxControl(
return (
<BaseControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
__associatedWPComponentName="CheckboxControl"
Copy link
Member

Choose a reason for hiding this comment

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

We're adding the __associatedWPComponentName prop only to components that can disable the __nextHasNoMarginBottom right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct 👍

label={ heading }
id={ id }
help={
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/checkbox-control/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export const Default: StoryFn< typeof CheckboxControl > = DefaultTemplate.bind(
{}
);
Default.args = {
__nextHasNoMarginBottom: true,
label: 'Is author',
help: 'Is the user an author or not?',
};
Expand Down
9 changes: 8 additions & 1 deletion packages/components/src/checkbox-control/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,20 @@ const noop = () => {};
const getInput = () => screen.getByRole( 'checkbox' ) as HTMLInputElement;

const CheckboxControl = ( props: Omit< CheckboxControlProps, 'onChange' > ) => {
return <BaseCheckboxControl onChange={ noop } { ...props } />;
return (
<BaseCheckboxControl
onChange={ noop }
{ ...props }
__nextHasNoMarginBottom
/>
);
};

const ControlledCheckboxControl = ( { onChange }: CheckboxControlProps ) => {
const [ isChecked, setChecked ] = useState( false );
return (
<BaseCheckboxControl
__nextHasNoMarginBottom
checked={ isChecked }
onChange={ ( value ) => {
setChecked( value );
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/combobox-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ function ComboboxControl( props: ComboboxControlProps ) {
<DetectOutside onFocusOutside={ onFocusOutside }>
<BaseControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
__associatedWPComponentName="ComboboxControl"
className={ clsx( className, 'components-combobox-control' ) }
label={ label }
id={ `components-form-token-input-${ instanceId }` }
Expand Down
7 changes: 3 additions & 4 deletions packages/components/src/combobox-control/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const Template: StoryFn< typeof ComboboxControl > = ( {
};
export const Default = Template.bind( {} );
Default.args = {
__nextHasNoMarginBottom: true,
allowReset: false,
label: 'Select a country',
options: countryOptions,
Expand Down Expand Up @@ -135,8 +136,7 @@ const optionsWithDisabledOptions = countryOptions.map( ( option, index ) => ( {
} ) );

WithDisabledOptions.args = {
allowReset: false,
label: 'Select a country',
...Default.args,
options: optionsWithDisabledOptions,
};

Expand All @@ -148,8 +148,7 @@ WithDisabledOptions.args = {
export const NotExpandOnFocus = Template.bind( {} );

NotExpandOnFocus.args = {
allowReset: false,
label: 'Select a country',
...Default.args,
options: countryOptions,
expandOnFocus: false,
};
6 changes: 5 additions & 1 deletion packages/components/src/combobox-control/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useState } from '@wordpress/element';
/**
* Internal dependencies
*/
import ComboboxControl from '..';
import _ComboboxControl from '..';
import type { ComboboxControlOption, ComboboxControlProps } from '../types';

const timezones = [
Expand Down Expand Up @@ -57,6 +57,10 @@ const getAllOptions = () => screen.getAllByRole( 'option' );
const getOptionSearchString = ( option: ComboboxControlOption ) =>
option.label.substring( 0, 11 );

const ComboboxControl = ( props: ComboboxControlProps ) => {
return <_ComboboxControl { ...props } __nextHasNoMarginBottom />;
};

const ControlledComboboxControl = ( {
value: valueProp,
onChange,
Expand Down
34 changes: 24 additions & 10 deletions packages/components/src/dimension-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ import SelectControl from '../select-control';
import sizesTable, { findSizeBySlug } from './sizes';
import type { DimensionControlProps, Size } from './types';
import type { SelectControlSingleSelectionProps } from '../select-control/types';
import { ContextSystemProvider } from '../context';

const CONTEXT_VALUE = {
BaseControl: {
// Temporary during deprecation grace period: Overrides the underlying `__associatedWPComponentName`
// via the context system to override the value set by SelectControl.
_overrides: { __associatedWPComponentName: 'DimensionControl' },
},
};

/**
* `DimensionControl` is a component designed to provide a UI to control spacing and/or dimensions.
Expand Down Expand Up @@ -87,16 +96,21 @@ export function DimensionControl( props: DimensionControlProps ) {
);

return (
<SelectControl
__next40pxDefaultSize={ __next40pxDefaultSize }
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
className={ clsx( className, 'block-editor-dimension-control' ) }
label={ selectLabel }
hideLabelFromVision={ false }
value={ value }
onChange={ onChangeSpacingSize }
options={ formatSizesAsOptions( sizes ) }
/>
<ContextSystemProvider value={ CONTEXT_VALUE }>
<SelectControl
__next40pxDefaultSize={ __next40pxDefaultSize }
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
className={ clsx(
className,
'block-editor-dimension-control'
) }
label={ selectLabel }
hideLabelFromVision={ false }
value={ value }
onChange={ onChangeSpacingSize }
options={ formatSizesAsOptions( sizes ) }
/>
</ContextSystemProvider>
);
}

Expand Down
1 change: 1 addition & 0 deletions packages/components/src/dimension-control/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const Template: StoryFn< typeof DimensionControl > = ( args ) => (
export const Default = Template.bind( {} );

Default.args = {
__nextHasNoMarginBottom: true,
label: 'Please select a size',
sizes,
};
16 changes: 0 additions & 16 deletions packages/components/src/dimension-control/test/__snapshots__/index.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ exports[`DimensionControl rendering renders with custom sizes 1`] = `
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down Expand Up @@ -299,10 +295,6 @@ exports[`DimensionControl rendering renders with defaults 1`] = `
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down Expand Up @@ -595,10 +587,6 @@ exports[`DimensionControl rendering renders with icon and custom icon label 1`]
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down Expand Up @@ -903,10 +891,6 @@ exports[`DimensionControl rendering renders with icon and default icon label 1`]
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/dimension-control/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import { plus } from '@wordpress/icons';
/**
* Internal dependencies
*/
import { DimensionControl } from '../';
import { DimensionControl as _DimensionControl } from '../';

const DimensionControl = ( props ) => {
return <_DimensionControl { ...props } __nextHasNoMarginBottom />;
};

describe( 'DimensionControl', () => {
const onChangeHandler = jest.fn();
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/focal-point-picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ export function FocalPointPicker( {
<BaseControl
{ ...restProps }
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
__associatedWPComponentName="FocalPointPicker"
label={ label }
id={ id }
help={ help }
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/focal-point-picker/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ const Template: StoryFn< typeof FocalPointPicker > = ( {
};

export const Default = Template.bind( {} );
Default.args = {
__nextHasNoMarginBottom: true,
};

export const Image = Template.bind( {} );
Image.args = {
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/focal-point-picker/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ import userEvent from '@testing-library/user-event';
/**
* Internal dependencies
*/
import Picker from '..';
import _Picker from '..';
import type { FocalPointPickerProps } from '../types';

type Log = { name: string; args: unknown[] };
type EventLogger = ( name: string, args: unknown[] ) => void;

const Picker = ( props: React.ComponentProps< typeof _Picker > ) => {
return <_Picker { ...props } __nextHasNoMarginBottom />;
};

const props: FocalPointPickerProps = {
onChange: jest.fn(),
url: 'test-url',
Expand Down
1 change: 1 addition & 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 @@ -213,6 +213,7 @@ function UnforwardedRangeControl(
return (
<BaseControl
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
__associatedWPComponentName="RangeControl"
className={ classes }
label={ label }
hideLabelFromVision={ hideLabelFromVision }
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/range-control/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const Template: StoryFn< typeof RangeControl > = ( { onChange, ...args } ) => {

export const Default: StoryFn< typeof RangeControl > = Template.bind( {} );
Default.args = {
__nextHasNoMarginBottom: true,
help: 'Please select how transparent you would like this.',
initialPosition: 50,
label: 'Opacity',
Expand Down Expand Up @@ -104,6 +105,7 @@ export const WithAnyStep: StoryFn< typeof RangeControl > = ( {
);
};
WithAnyStep.args = {
__nextHasNoMarginBottom: true,
label: 'Brightness',
step: 'any',
};
Expand Down Expand Up @@ -167,6 +169,7 @@ export const WithIntegerStepAndMarks: StoryFn< typeof RangeControl > =
MarkTemplate.bind( {} );

WithIntegerStepAndMarks.args = {
__nextHasNoMarginBottom: true,
Copy link
Member

Choose a reason for hiding this comment

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

Ah, would have been nice to have an object to declare common props. Not a blocker of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha yes, I considered abstracting it but decided against doing it here because it would be annoying to review it in this big PR 😂

label: 'Integer Step',
marks: marksBase,
max: 10,
Expand All @@ -183,6 +186,7 @@ export const WithDecimalStepAndMarks: StoryFn< typeof RangeControl > =
MarkTemplate.bind( {} );

WithDecimalStepAndMarks.args = {
__nextHasNoMarginBottom: true,
marks: [
...marksBase,
{ value: 3.5, label: '3.5' },
Expand All @@ -202,6 +206,7 @@ export const WithNegativeMinimumAndMarks: StoryFn< typeof RangeControl > =
MarkTemplate.bind( {} );

WithNegativeMinimumAndMarks.args = {
__nextHasNoMarginBottom: true,
marks: marksWithNegatives,
max: 10,
min: -10,
Expand All @@ -217,6 +222,7 @@ export const WithNegativeRangeAndMarks: StoryFn< typeof RangeControl > =
MarkTemplate.bind( {} );

WithNegativeRangeAndMarks.args = {
__nextHasNoMarginBottom: true,
marks: marksWithNegatives,
max: -1,
min: -10,
Expand All @@ -232,6 +238,7 @@ export const WithAnyStepAndMarks: StoryFn< typeof RangeControl > =
MarkTemplate.bind( {} );

WithAnyStepAndMarks.args = {
__nextHasNoMarginBottom: true,
marks: marksBase,
max: 10,
min: 0,
Expand Down
Loading
Loading