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

Update control labels to the new uppercase styles #42789

Merged
merged 10 commits into from
Aug 5, 2022
Merged

Update control labels to the new uppercase styles #42789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
167fac2
Refactor label styles for BaseControl and InputControl
mirka Jul 28, 2022
c6a4605
Make BaseControl.VisualLabel props polymorphic
mirka Jul 28, 2022
932974d
Clean up label styles across codebase
mirka Jul 28, 2022
3cbaf52
Update changelog
mirka Aug 1, 2022
03af77e
BaseControl.VisualLabel: Pass props through
mirka Aug 1, 2022
46f4d9e
Remove unnecessary `className` type
mirka Aug 1, 2022
486f9a3
Update snapshots
mirka Aug 1, 2022
32fd29f
Merge branch 'trunk' into update/control-labels
mirka Aug 2, 2022
c536e8e
CustomGradientPicker: Remove duplicate label styles
mirka Aug 2, 2022
68e560f
Merge branch 'trunk' into update/control-labels
mirka Aug 3, 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
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/border-radius-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import {
BaseControl,
RangeControl,
__experimentalParseQuantityAndUnitFromRawValue as parseQuantityAndUnitFromRawValue,
__experimentalUseCustomUnits as useCustomUnits,
Expand Down Expand Up @@ -70,7 +71,9 @@ export default function BorderRadiusControl( { onChange, values } ) {

return (
<fieldset className="components-border-radius-control">
<legend>{ __( 'Radius' ) }</legend>
<BaseControl.VisualLabel as="legend">
{ __( 'Radius' ) }
</BaseControl.VisualLabel>
<div className="components-border-radius-control__wrapper">
{ isLinked ? (
<>
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,13 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
}

.emotion-6 {
font-size: 11px;
font-weight: 500;
line-height: 1.4;
text-transform: uppercase;
display: inline-block;
margin-bottom: calc(4px * 2);
padding: 0;
}

.emotion-8 {
Expand Down
6 changes: 4 additions & 2 deletions packages/block-editor/src/components/text-decoration-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { BaseControl, Button } from '@wordpress/components';
import { formatStrikethrough, formatUnderline } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';

Expand Down Expand Up @@ -30,7 +30,9 @@ const TEXT_DECORATIONS = [
export default function TextDecorationControl( { value, onChange } ) {
return (
<fieldset className="block-editor-text-decoration-control">
<legend>{ __( 'Decoration' ) }</legend>
<BaseControl.VisualLabel as="legend">
{ __( 'Decoration' ) }
</BaseControl.VisualLabel>
<div className="block-editor-text-decoration-control__buttons">
{ TEXT_DECORATIONS.map( ( textDecoration ) => {
return (
Expand Down
6 changes: 4 additions & 2 deletions packages/block-editor/src/components/text-transform-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { BaseControl, Button } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import {
formatCapitalize,
Expand Down Expand Up @@ -39,7 +39,9 @@ const TEXT_TRANSFORMS = [
export default function TextTransformControl( { value, onChange } ) {
return (
<fieldset className="block-editor-text-transform-control">
<legend>{ __( 'Letter case' ) }</legend>
<BaseControl.VisualLabel as="legend">
{ __( 'Letter case' ) }
</BaseControl.VisualLabel>
<div className="block-editor-text-transform-control__buttons">
{ TEXT_TRANSFORMS.map( ( textTransform ) => {
return (
Expand Down
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 @@ -15,6 +15,7 @@

- Add `box-sizing` reset style mixin to utils ([#42754](https://github.com/WordPress/gutenberg/pull/42754)).
- `ResizableBox`: Make tooltip background match `Tooltip` component's ([#42800](https://github.com/WordPress/gutenberg/pull/42800)).
- 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)).

Expand Down
8 changes: 5 additions & 3 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 @@ -2,7 +2,6 @@
* External dependencies
*/
import classnames from 'classnames';
import type { FunctionComponent } from 'react';

/**
* Internal dependencies
Expand All @@ -16,6 +15,7 @@ import {
StyledHelp,
StyledVisualLabel,
} from './styles/base-control-styles';
import type { WordPressComponentProps } from '../ui/context';

/**
* `BaseControl` is a component used to generate labels and help text for components handling user inputs.
Expand Down Expand Up @@ -104,12 +104,14 @@ export const BaseControl = ( {
* </BaseControl>
* );
*/
export const VisualLabel: FunctionComponent< BaseControlVisualLabelProps > = ( {
export const VisualLabel = ( {
className,
children,
} ) => {
...props
}: WordPressComponentProps< BaseControlVisualLabelProps, 'span' > ) => {
mirka marked this conversation as resolved.
Show resolved Hide resolved
return (
<StyledVisualLabel
{ ...props }
className={ classnames(
'components-base-control__label',
className
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/base-control/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ WithHelpText.args = {
export const WithVisualLabel: ComponentStory< typeof BaseControl > = (
props
) => {
// @ts-expect-error - Unclear how to fix, see also https://github.com/WordPress/gutenberg/pull/39468#discussion_r827150516
BaseControl.VisualLabel.displayName = 'BaseControl.VisualLabel';
Comment on lines +63 to 64
Copy link
Member Author

Choose a reason for hiding this comment

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

Side effect of the changing the TS types for VisualLabel! We might want to look into a solution if we encounter this more.


return (
Expand Down
12 changes: 7 additions & 5 deletions packages/components/src/base-control/styles/base-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 { css } from '@emotion/react';
/**
* Internal dependencies
*/
import { font, COLORS, boxSizingReset } from '../../utils';
import { baseLabelTypography, boxSizingReset, font, COLORS } from '../../utils';
import { space } from '../../ui/utils/space';

export const Wrapper = styled.div`
Expand Down Expand Up @@ -35,19 +35,21 @@ export const StyledField = styled.div`
`;

const labelStyles = css`
${ baseLabelTypography };

display: inline-block;
margin-bottom: ${ space( 2 ) };
`;

export const StyledLabel = styled.label`
${ labelStyles }
/**
* Removes Chrome/Safari/Firefox user agent stylesheet padding from
* StyledLabel when it is rendered as a legend.
*/
padding: 0;
Comment on lines 42 to 46
Copy link
Member Author

Choose a reason for hiding this comment

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

This legend reset has been moved from being included only in StyledLabel, to being shared between both StyledLabel and StyledVisualLabel . This is because StyledVisualLabel (a.k.a. BaseControl.VisualLabel) is also likely to be used as="legend".

`;

export const StyledLabel = styled.label`
${ labelStyles }
`;

const deprecatedMarginHelp = ( { __nextHasNoMarginBottom = false } ) => {
return (
! __nextHasNoMarginBottom &&
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/base-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,5 @@ export type BaseControlProps = {
};

export type BaseControlVisualLabelProps = {
className?: string;
children: ReactNode;
};
9 changes: 3 additions & 6 deletions packages/components/src/box-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { BaseControl } from '../base-control';
import Button from '../button';
import { FlexItem, FlexBlock } from '../flex';
import AllInputControl from './all-input-control';
import InputControls from './input-controls';
import AxialInputControls from './axial-input-controls';
import BoxControlIcon from './icon';
import { Text } from '../text';
import LinkedButton from './linked-button';
import {
Root,
Expand Down Expand Up @@ -120,12 +120,9 @@ export default function BoxControl( {
<Root id={ id } role="group" aria-labelledby={ headingId }>
<Header className="component-box-control__header">
<FlexItem>
<Text
id={ headingId }
className="component-box-control__label"
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it we are ok removing the components-box-control__label class here given the BoxControl is still exported as experimental?

I ask, as later in this PR, we keep the components-form-token-field__label class desite removing the styles for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! I am actually quite inclined to remove unused CSS classes even if they are in stable components, unless it's a class that we can imagine a lot of third-party consumers to be overriding in a crucial way (e.g. components-popover__content). (@ciampo Does that sound about right or should we be more conservative about it?)

The reason I left components-form-token-field__label intact is more because it's being used as a selector in an old e2e test 🙃

Copy link
Contributor

@ciampo ciampo Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the late answer, I'm currently a bit behind on my review queue 😅

I believe we should address this situation in a more general way (and potentially even add a paragraph about the strategy we decide to adopt to the contributors' guidelines).

In general. I'd love to remove hardcoded classnames — in practice, it may cause breakage for consumers that rely on them.

Should we consider the removal of hardcoded classnames a breaking change?

  • On one hand, hardcoded classnames are not part of the public APIs of the components (except for a these 4 components 🤦 ) — and so, technically it would not be a breaking change
  • On the other hand, hardcoded classnames have been used outside of the package for years (both inside and outside the Gutenberg repo).

For example, even in the case of the removal component-box-control__label, we're not 100% guaranteed that it won't cause breakage to some external consumers.

In general, I believe we should at least aim at removing all instances of component-* classnames outside of the components' package. After that, depending on how conservative we want to be, we could:

  • also delete the hardcoded classnames in the source components
  • keep them as "legacy" support, but add a ESLINT rule forbidding their usage in Gutenberg (although this won't stop external consumers from using them)

What do folks think?

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 believe we should address this situation in a more general way (and potentially even add a paragraph about the strategy we decide to adopt to the contributors' guidelines).

Absolutely, I don't think it's explicitly written anywhere.

Should we consider the removal of hardcoded classnames a breaking change?

Could it cause breakage? Yes. Should we consider it a Breaking Change? No.

I was a consumer of the wp-components package before coming to the maintainer side, and I was never under the impression that CSS classnames (or styling, or DOM structure, for that matter) were guaranteed not to change. I think most people realize that overriding internal styling is fragile, and that it's their responsibility to protect against breakage if they are going to do so. Unless these classnames are publicly documented, as in the examples you raised 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I agree with the notion that while it might cause breakage, we shouldn't necessarily consider it a breaking change. That said, I think communication and documentation will be key to maintaining as much good will as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with both of you. I opened #43083 to keep track of this task separately

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, the project does have existing documentation about backwards compatibility around class names and DOM updates.

Class names and DOM nodes used inside the tree of React components are not considered part of the public API and can be modified.

Changes to these should be done with caution as it can affect the styling and behavior of third-party code (Even if they should not rely on these in the first place). Keep the old ones if possible. If not, document the changes and write a dev note.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @nerrad ! Seems to align with our thoughts above

>
<BaseControl.VisualLabel id={ headingId }>
{ label }
</Text>
</BaseControl.VisualLabel>
</FlexItem>
{ allowReset && (
<FlexItem>
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/box-control/styles/box-control-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import styled from '@emotion/styled';
*/
import { Flex } from '../../flex';
import BaseUnitControl from '../../unit-control';
import { COLORS, rtl } from '../../utils';
import { rtl } from '../../utils';

export const Root = styled.div`
box-sizing: border-box;
Expand All @@ -18,7 +18,6 @@ export const Root = styled.div`
`;

export const Header = styled( Flex )`
color: ${ COLORS.ui.label };
margin-bottom: 8px;
`;

Expand Down
12 changes: 0 additions & 12 deletions packages/components/src/custom-gradient-picker/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,3 @@ $components-custom-gradient-picker__padding: $grid-unit-20; // 48px container, 1
}
}
}

// All these styles should be made generic and changed on the inputs for all components.
.components-custom-gradient-picker {
.components-input-control__label {
line-height: 1;
}
label {
text-transform: uppercase;
font-size: 11px;
font-weight: 500;
}
}
5 changes: 3 additions & 2 deletions packages/components/src/custom-select-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { useCallback, useState } from '@wordpress/element';
import { VisuallyHidden } from '../';
import { Select as SelectControlSelect } from '../select-control/styles/select-control-styles';
import InputBase from '../input-control/input-base';
import { StyledLabel } from '../base-control/styles/base-control-styles';

const itemToString = ( item ) => item?.name;
// This is needed so that in Windows, where
Expand Down Expand Up @@ -138,13 +139,13 @@ export default function CustomSelectControl( {
</VisuallyHidden>
) : (
/* eslint-disable-next-line jsx-a11y/label-has-associated-control, jsx-a11y/label-has-for */
<label
<StyledLabel
{ ...getLabelProps( {
className: 'components-custom-select-control__label',
} ) }
>
{ label }
</label>
</StyledLabel>
) }
<InputBase
isFocused={ isOpen || isFocused }
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/custom-select-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
font-size: $default-font-size;
}

.components-custom-select-control__label {
display: block;
margin-bottom: $grid-unit-10;
}

.components-custom-select-control__button {
position: relative;
text-align: left;
Expand Down
12 changes: 7 additions & 5 deletions packages/components/src/date-time/time/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import BaseControl from '../../base-control';
import Button from '../../button';
import ButtonGroup from '../../button-group';
import TimeZone from './timezone';
import type { TimePickerProps } from '../types';
import {
Wrapper,
Fieldset,
Legend,
HoursInput,
TimeSeparator,
MinutesInput,
Expand Down Expand Up @@ -220,11 +220,12 @@ export function TimePicker( {
className="components-datetime__time" // Unused, for backwards compatibility.
>
<Fieldset>
<Legend
<BaseControl.VisualLabel
as="legend"
className="components-datetime__time-legend" // Unused, for backwards compatibility.
>
{ __( 'Time' ) }
</Legend>
</BaseControl.VisualLabel>
<HStack
className="components-datetime__time-wrapper" // Unused, for backwards compatibility.
>
Expand Down Expand Up @@ -309,11 +310,12 @@ export function TimePicker( {
</HStack>
</Fieldset>
<Fieldset>
<Legend
<BaseControl.VisualLabel
as="legend"
className="components-datetime__time-legend" // Unused, for backwards compatibility.
>
{ __( 'Date' ) }
</Legend>
</BaseControl.VisualLabel>
<HStack
className="components-datetime__time-wrapper" // Unused, for backwards compatibility.
>
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/date-time/time/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ export const Fieldset = styled.fieldset`
}
`;

export const Legend = styled.legend`
margin-bottom: ${ space( 2 ) };
padding: 0;
`;

export const TimeWrapper = styled.div`
direction: ltr;
display: flex;
Expand Down
17 changes: 11 additions & 6 deletions packages/components/src/font-size-picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { useState, useMemo, forwardRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import { BaseControl } from '../base-control';
import Button from '../button';
import RangeControl from '../range-control';
import { Flex, FlexItem } from '../flex';
Expand Down Expand Up @@ -131,12 +132,16 @@ function FontSizePicker(
className={ `${ baseClassName }__header` }
>
<FlexItem>
{ __( 'Size' ) }
{ headerHint && (
<span className={ `${ baseClassName }__header__hint` }>
{ headerHint }
</span>
) }
<BaseControl.VisualLabel>
mirka marked this conversation as resolved.
Show resolved Hide resolved
{ __( 'Size' ) }
{ headerHint && (
<span
className={ `${ baseClassName }__header__hint` }
>
{ headerHint }
</span>
) }
</BaseControl.VisualLabel>
</FlexItem>
{ ! disableCustomFontSizes && (
<FlexItem>
Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/form-token-field/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { TokensAndInputWrapperFlex } from './styles';
import SuggestionsList from './suggestions-list';
import type { FormTokenFieldProps, TokenItem } from './types';
import { FlexItem } from '../flex';
import { StyledLabel } from '../base-control/styles/base-control-styles';

const identity = ( value: string ) => value;

Expand Down Expand Up @@ -659,12 +660,12 @@ export function FormTokenField( props: FormTokenFieldProps ) {
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div { ...tokenFieldProps }>
<label
<StyledLabel
htmlFor={ `components-form-token-input-${ instanceId }` }
className="components-form-token-field__label"
>
{ label }
</label>
</StyledLabel>
<div
ref={ tokensAndInput }
className={ classes }
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/form-token-field/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@
}
}

.components-form-token-field__label {
display: inline-block;
margin-bottom: $grid-unit-10;
}

.components-form-token-field__help {
font-size: $helptext-font-size;
font-style: normal;
Expand Down
Loading