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

Jump to bottom

BaseControl: Add opt-in prop for margin-free styles #39325

Merged
merged 7 commits into from
Mar 14, 2022
Merged

BaseControl: Add opt-in prop for margin-free styles #39325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
349ccc4
BaseControl: Add opt-in prop for margin-free styles
mirka Mar 9, 2022
15095fb
Fix story
mirka Mar 9, 2022
7560629
Clear out margin bottom for help text
mirka Mar 9, 2022
dfcaa30
Add changelog entry
mirka Mar 9, 2022
6d7d2f1
Fixup readme
mirka Mar 10, 2022
9c24d1d
Merge branch 'trunk' into add/base-control-no-margin
mirka Mar 10, 2022
c0e010c
Merge branch 'trunk' into add/base-control-no-margin
mirka Mar 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 @@ -10,6 +10,7 @@
- `InputControl`: Allow `onBlur` for empty values to commit the change when `isPressEnterToChange` is true, and move reset behavior to the ESCAPE key. ([#39109](https://github.com/WordPress/gutenberg/pull/39109)).
- `TreeGrid`: Add tests for Home/End keyboard navigation. Add `onFocusRow` callback for Home/End keyboard navigation, this was missed in the implementation PR. Modify test for expanding/collapsing a row as row 1 implements this now. Update README with latest changes. ([#39302](https://github.com/WordPress/gutenberg/pull/39302))
- `ToggleGroupControlOption`: Calculate width from button content and remove `LabelPlaceholderView` ([#39345](https://github.com/WordPress/gutenberg/pull/39345))
- `BaseControl`: Add `__nextHasNoMarginBottom` prop for opting into the new margin-free styles ([#39325](https://github.com/WordPress/gutenberg/pull/39325)).

### Bug Fix

Expand Down
10 changes: 9 additions & 1 deletion packages/components/src/base-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Render a BaseControl for a textarea input:
import { BaseControl } from '@wordpress/components';

const MyBaseControl = () => (
<BaseControl id="textarea-1" label="Text" help="Enter some text">
<BaseControl id="textarea-1" label="Text" help="Enter some text" __nextHasNoMarginBottom={ true }>
<textarea id="textarea-1" />
</BaseControl>
);
Expand Down Expand Up @@ -63,6 +63,14 @@ The content to be displayed within the BaseControl.
- Type: `Element`
- Required: Yes

### __nextHasNoMarginBottom

Start opting into the new margin-free styles that will become the default in a future version.

- Type: `Boolean`
- Required: No
- Default: `false`

## BaseControl.VisualLabel

`BaseControl.VisualLabel` component is used to render a purely visual label inside a `BaseControl` component.
Expand Down
33 changes: 20 additions & 13 deletions packages/components/src/base-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,27 @@ import {

/**
* @typedef Props
* @property {string} [id] The id of the element to which labels and help text are being generated.
* That element should be passed as a child.
* @property {import('react').ReactNode} help If this property is added, a help text will be
* generated using help property as the content.
* @property {import('react').ReactNode} [label] If this property is added, a label will be generated
* using label property as the content.
* @property {boolean} [hideLabelFromVision] If true, the label will only be visible to screen readers.
* @property {string} [className] The class that will be added with "components-base-control" to the
* classes of the wrapper div. If no className is passed only
* components-base-control is used.
* @property {import('react').ReactNode} [children] The content to be displayed within
* the BaseControl.
* @property {boolean} [__nextHasNoMarginBottom] Start opting into the new margin-free styles that will become the default in a future version.
* @property {string} [id] The id of the element to which labels and help text are being generated.
* That element should be passed as a child.
* @property {import('react').ReactNode} help If this property is added, a help text will be
* generated using help property as the content.
* @property {import('react').ReactNode} [label] If this property is added, a label will be generated
* using label property as the content.
* @property {boolean} [hideLabelFromVision] If true, the label will only be visible to screen readers.
* @property {string} [className] The class that will be added with "components-base-control" to the
* classes of the wrapper div. If no className is passed only
* components-base-control is used.
* @property {import('react').ReactNode} [children] The content to be displayed within
* the BaseControl.
*/

/**
* @param {Props} props
* @return {JSX.Element} Element
*/
function BaseControl( {
__nextHasNoMarginBottom = false,
id,
label,
hideLabelFromVision,
Expand All @@ -47,7 +49,11 @@ function BaseControl( {
<Wrapper
className={ classnames( 'components-base-control', className ) }
>
<StyledField className="components-base-control__field">
<StyledField
className="components-base-control__field"
// TODO: Official deprecation for this should start after all internal usages have been migrated
Copy link
Contributor

Choose a reason for hiding this comment

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

Official deprecation for this should start after all internal usages have been migrated

👍

__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
>
{ label &&
id &&
( hideLabelFromVision ? (
Expand Down Expand Up @@ -77,6 +83,7 @@ function BaseControl( {
<StyledHelp
id={ id ? id + '__help' : undefined }
className="components-base-control__help"
__nextHasNoMarginBottom={ __nextHasNoMarginBottom }
>
{ help }
</StyledHelp>
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/base-control/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import BaseControl from '../';
import Button from '../../button';
import { Spacer } from '../../spacer';
import TextareaControl from '../../textarea-control';

export default {
title: 'Components/BaseControl',
Expand All @@ -15,13 +14,14 @@ export default {
const BaseControlWithTextarea = ( { id, ...props } ) => {
return (
<BaseControl id={ id } { ...props }>
<TextareaControl id={ id } />
<textarea style={ { display: 'block' } } id={ id } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason why we'd swap the library's component with the vanilla HTML one? Do you think this should be a pattern that we apply to stories across the package?

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 so when I was working on this PR there was a weird issue I was debugging for half an hour, and I finally realized that TextareaControl is already built with BaseControl 🤦🏻‍♀️ 15095fb

</BaseControl>
);
};

export const Default = BaseControlWithTextarea.bind( {} );
Default.args = {
__nextHasNoMarginBottom: true,
id: 'textarea-1',
label: '',
hideLabelFromVision: false,
Expand Down
24 changes: 23 additions & 1 deletion packages/components/src/base-control/styles/base-control-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,17 @@ export const Wrapper = styled.div`
font-size: ${ font( 'default.fontSize' ) };
`;

const deprecatedMarginField = ( { __nextHasNoMarginBottom = false } ) => {
return (
! __nextHasNoMarginBottom &&
css`
margin-bottom: ${ space( 2 ) };
`
);
};

export const StyledField = styled.div`
margin-bottom: ${ space( 2 ) };
${ deprecatedMarginField }

.components-panel__row & {
margin-bottom: inherit;
Expand All @@ -32,10 +41,23 @@ export const StyledLabel = styled.label`
${ labelStyles }
`;

const deprecatedMarginHelp = ( { __nextHasNoMarginBottom = false } ) => {
return (
! __nextHasNoMarginBottom &&
css`
margin-bottom: revert;
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL (or rediscovered) revert — very cool!

`
);
};

export const StyledHelp = styled.p`
margin-top: ${ space( 2 ) };
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 can go ahead and add this margin-top regardless of whether StyledField still has a margin-bottom, because this margin-top will collapse together with StyledField's margin-bottom anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

margin-bottom: 0;
font-size: ${ font( 'helpText.fontSize' ) };
font-style: normal;
color: ${ COLORS.mediumGray.text };

${ deprecatedMarginHelp }
`;

export const StyledVisualLabel = styled.span`
Expand Down