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

Update HeightControl component to label inputs #63761

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Update HeightControl component to label inputs #63761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e701a2
Update HeightControl component to label inputs
andrew-palapa Feb 27, 2023
25bc9fe
Changelog entry.
alexstine Mar 4, 2024
9a97781
Tweak instance ID prefix
ciampo Jul 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
15 changes: 12 additions & 3 deletions packages/block-editor/src/components/height-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { useMemo } from '@wordpress/element';
import {
BaseControl,
Expand Down Expand Up @@ -68,6 +69,10 @@ export default function HeightControl( {
value,
} ) {
const customRangeValue = parseFloat( value );
const id = useInstanceId( HeightControl, 'block-editor-height-control' );
const labelId = `${ id }__label`;
const inputId = `${ id }__input`;
const rangeId = `${ id }__range`;

const [ availableUnits ] = useSettings( 'spacing.units' );
const units = useCustomUnits( {
Expand Down Expand Up @@ -144,13 +149,14 @@ export default function HeightControl( {
};

return (
<fieldset className="block-editor-height-control">
<BaseControl.VisualLabel as="legend">
<div className="block-editor-height-control" id={ id }>
<BaseControl.VisualLabel as="label" for={ inputId } id={ labelId }>
Copy link
Contributor

@afercia afercia Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess this for should have been htmlFor. Getting this in the condole:

Warning: Invalid DOM property `for`. Did you mean `htmlFor`?
at label
at div
at HeightControl
etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - I must have let my guard down knowing that this PR had been reviewed before. Thank you for pointing it out

{ label }
</BaseControl.VisualLabel>
<Flex>
<FlexItem isBlock>
<UnitControl
id={ inputId }
value={ value }
units={ units }
onChange={ onChange }
Expand All @@ -164,6 +170,9 @@ export default function HeightControl( {
<FlexItem isBlock>
<Spacer marginX={ 2 } marginBottom={ 0 }>
<RangeControl
aria-controls={ inputId }
aria-labelledby={ labelId }
id={ rangeId }
value={ customRangeValue }
min={ 0 }
max={
Expand All @@ -183,6 +192,6 @@ export default function HeightControl( {
</Spacer>
</FlexItem>
</Flex>
</fieldset>
</div>
);
}
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 @@ -18,6 +18,7 @@
- `CustomSelectControl`: Stabilize `__experimentalShowSelectedHint` and `options[]. __experimentalHint` props ([#63248](https://github.com/WordPress/gutenberg/pull/63248)).
- `SelectControl`: Add `"minimal"` variant ([#63265](https://github.com/WordPress/gutenberg/pull/63265)).
- `FontSizePicker`: tidy up internal logic ([#63553](https://github.com/WordPress/gutenberg/pull/63553)).
- `RangeControl`: Allow external `id` prop ([#63761](https://github.com/WordPress/gutenberg/pull/63761)).

### Internal

Expand Down
15 changes: 11 additions & 4 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 @@ -41,6 +41,15 @@ import { space } from '../utils/space';

const noop = () => {};

function useUniqueId( idProp?: string ) {
const id = useInstanceId(
UnforwardedRangeControl,
'inspector-range-control'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what this component has to do with inspector though. Maybe not much anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the instance ID prefix that many components in the package adopt 😓 Not sure how easy it would be to refactor away from it, but probably not in the scope for this PR ('inspector-range-control' is already used in trunk and in this PR it was moved to the useUniqueId function)

);

return idProp || id;
Copy link
Member

Choose a reason for hiding this comment

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

Do we mean to use || or ???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that same question when reviewing the original PR, and at the time we agreed that || was a good choice because empty strings would not make a good id. #48498 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Also, reminder that the useInstanceId() util supports preferredId as the third argument. (I guess it didn't before, hence the custom logic like this in some of these older components.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about it — thank you for pointing it out!

}

function UnforwardedRangeControl(
props: WordPressComponentProps< RangeControlProps, 'input', false >,
forwardedRef: ForwardedRef< HTMLInputElement >
Expand All @@ -56,6 +65,7 @@ function UnforwardedRangeControl(
disabled = false,
help,
hideLabelFromVision = false,
id: idProp,
initialPosition,
isShiftStepEnabled = true,
label,
Expand Down Expand Up @@ -123,10 +133,7 @@ function UnforwardedRangeControl(
!! marks && 'is-marked'
);

const id = useInstanceId(
UnforwardedRangeControl,
'inspector-range-control'
);
const id = useUniqueId( idProp );
const describedBy = !! help ? `${ id }__help` : undefined;
const enableTooltip = hasTooltip !== false && Number.isFinite( value );

Expand Down
Loading