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.

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

Add inside label position to InputControl #55977

Closed
wants to merge 4 commits into from

Conversation

andrewhayward
Copy link
Contributor

What?

This change adds a new LabelPosition value of inside to InputControl, allowing consumers to render the label visually inside the field.

An input field, with a label inside the containing border

Resolves #55963

Why?

As noted in #55963, a use case has come up where a field needs to visually contain its label. This is not currently possible, and as such encourages consumers to use the prefix prop instead. This not only means that the field is less likely to have an accessible name, as the label prop may well get forgotten, but the visual label will not pass focus to the input.

How?

The change adds an additional value of inside to the LabelPosition enum. This is checked inside InputBase, which renders the label element inside as appropriate. Minor changes have been made to the CSS to accommodate this, but existing labels have not changed. Prefix values will also continue to work as before, appearing between any inside label and the input.

Testing Instructions

This shouldn't impact existing labels, as nothing around their implementation has changed. Prefixes shouldn't change either, although the CSS controlling their appearance has been updated slightly.

To test the inner label:

  1. Navigate to InputControl in Storybook (likely here).
  2. Confirm that setting labelPosition to inside works as expected
  3. Confirm that adding a prefix works as expected
  4. Confirm that having both doesn't break
  5. Confirm that the input field has an accessible name

Additionally:

  1. Navigate to SelectControl in Storybook (likely here)
  2. Confirm that setting labelPosition to inside works as expected

Testing Instructions for Keyboard

This change should make no difference to keyboard interaction. This can be confirmed by tabbing through the control, with and without an inside label. The focus should move to the input, as normal.

This change adds a new `LabelPosition` value of `inside` to `InputControl`, allowing consumers to render the label visually inside the field.

Resolves #55963
@andrewhayward andrewhayward requested a review from a team November 8, 2023 17:34
@andrewhayward andrewhayward added [Package] Components /packages/components [Type] Feature New feature to highlight in changelogs. [Type] Enhancement A suggestion for improvement. and removed [Type] Feature New feature to highlight in changelogs. labels Nov 8, 2023
@@ -33,3 +35,15 @@ export default function Label( {
</LabelWrapper>
);
}

export function InnerLabel( {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that, when the label position is set to inside but the label prop is still empty, the inner label component still renders an empty label element, which also causes the select element to shift slightly to the right.

I wonder if we should avoid rendering a label at all when its contents are empty?

andrewhayward pushed a commit that referenced this pull request Nov 9, 2023
All user inputs should have an accessible name, ideally provided by a visible label. This patch ensures that data views "in" filters semantically associate the visible label with the select input, ensuring that the input has an accessible name.

Resolves #55963
Closes #55977
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 9, 2023
Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

I had the same thought/observation as @ciampo regarding the shift with an empty inside label, but it tests well and looks good to me otherwise!

@ciampo
Copy link
Contributor

ciampo commented Nov 10, 2023

Overall, this is nice work! It probably needs some design feedback around the style of the label in the new inside position.

But since #56001 seems to tackle the immediate product need, we can probably pause work on this and resume if/when we'll be needing such feature at the components level

@andrewhayward
Copy link
Contributor Author

For the sake of completeness I've added some logic to check for the presence of a label value before rendering either the original label or the new inside version.

But to @ciampo's point, this can be put on hold until a wider use-case arises. Will work on #56001 to resolve the specific issue at hand.

Copy link

Flaky tests detected in cbe90bb.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6825085894
📝 Reported issues:

@andrewhayward andrewhayward added [Status] In discussion Used to indicate that an issue is in the process of being discussed and removed [Status] In Progress Tracking issues with work in progress labels Nov 10, 2023
@andrewhayward
Copy link
Contributor Author

Closing this, as it's no longer required. Should interest in #55963 resurface, this PR can be revisited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Status] In discussion Used to indicate that an issue is in the process of being discussed [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new LabelPosition of inside to InputControl
3 participants