gsoft-inc / sg-orbit

The design system for ShareGate web apps.
https://orbit.sharegate.design
Apache License 2.0
101 stars 37 forks source link

Fix "An input component must either have a 'aria-label' attribute..." error when a <label> is present #1230

Open tjosepo opened 1 year ago

tjosepo commented 1 year ago

Issue: #1229

Summary

Removes the warning "An input component must either have a 'aria-label' attribute..." when a <label> is present.

What I did

I moved the validation logic inside a useEffect and add a check for isNilOrEmpty(input.labels).

I had to modify isNilOrEmpty to support arrays (and NodeList). Instead of checking value === "", it checks value.length === 0 (this works for strings, arrays and NodeLists).

How to test

Unit tests were added.

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: 871a9a3bc562e90b2bab78307b5be2099369d55a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | -------------------- | ----- | | @orbit-ui/components | Patch | | @sharegate/orbit-ui | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 1 year ago

Deploy Preview for sg-orbit ready!

Name Link
Latest commit 871a9a3bc562e90b2bab78307b5be2099369d55a
Latest deploy log https://app.netlify.com/sites/sg-orbit/deploys/64f895583e67b00008156181
Deploy Preview https://deploy-preview-1230--sg-orbit.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 year ago

Deploy Preview for sg-storybook ready!

Name Link
Latest commit 871a9a3bc562e90b2bab78307b5be2099369d55a
Latest deploy log https://app.netlify.com/sites/sg-storybook/deploys/64f89558d8cf0700088e1614
Deploy Preview https://deploy-preview-1230--sg-storybook.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

patricklafrance commented 1 year ago

I am not sure I understand the use case correctly.

Does the following code would print a warning?

    <Field required>
        <Label>Username</Label>
        <TextInput placeholder="john@spacex.com" />
    </Field>

When there's a Field and a Label, the label id should be retrieved from the FieldContext by the following code of the Input component:

const [fieldProps] = useFieldInputProps();

fieldProps would then add to the Input props the aria-describedby and aria-labelledby properties, which should prevent the warning from being displayed.

From what I can see by the following test, the issue might be that the Input and the Label are not wrapped within a Field component.

tjosepo commented 1 year ago

Does the following code would print a warning?

✅ I confirm that using <Field> worked without printing a warning.

I'll add it to the test scenarios.


I didn't know the <Field> component automatically added the accessibility properties.

To me, it still feel like a bug because using a label with htmlFor is accessible HTML, yet Orbit logs an error.

<>
    <Label htmlFor="test">Label</Label>
    <TextInput id="test" />
</>

I understand that labels are this is not as easily verifyable as using aria-label, aria-labelledby, and placeholder, since this forces us to move the validation logic after the component has been rendered on the page. This prevents us from checking accessibility during SSR. Altough, it is just as unit-testable.

I think it's a valid trade-off.

We can also argue against this change if we prefer people to use the <Field> component instead of manually passing in a label with htmlFor. In this case, we should update the warning message to mention using the <Field> component.

patricklafrance commented 1 year ago

Thank for the reply @tjosepo. Yes, the intent is to always use the <Field> component to group together a Label and an Input. Great idea to update the warning message!

tjosepo commented 1 year ago

Alright! I'll update my PR and go for it.