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

Fix: input label accessibility #1518

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

rohanm-crest
Copy link
Contributor

Issue number: ADDON-68266

PR Type

What kind of change does this PR introduce?

  • Feature
  • Bug Fix
  • Refactoring (no functional or API changes)
  • Documentation Update
  • Maintenance (dependency updates, CI, etc.)

Summary

Changes

Please provide a summary of the changes.

User experience

Please describe the user experience before and after this change. Screenshots are welcome for additional context.

Checklist

If an item doesn't apply to your changes, leave it unchecked.

@rohanm-crest rohanm-crest changed the title Bugfix/input accessibility Fix: input label accessibility Jan 2, 2025
@@ -372,6 +372,7 @@ class BaseFormView extends PureComponent<BaseFormProps, BaseFormState> {
disabled: false,
error: false,
display: true,
id: `${e.field}-${e.label}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We might use createDOMID to make sure there is no illegal characters

Copy link
Contributor

Choose a reason for hiding this comment

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

Konrad showed there is no need to create id because ControlGroup generates the one

@@ -51,6 +51,7 @@ export interface ControlWrapperProps {
required?: boolean;
};
page?: string;
id?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id?: number;
id?: string;

@@ -49,6 +49,7 @@ export type ModifyFieldsOnValue = Array<ModificationCriteria>;

export interface BaseFormStateData {
[x: string]: {
id: number | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id: number | undefined;
id?: string;

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure though why number

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not work for any of oauth fields

Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget about tests. Although, adding tests exactly for this is extra, I would just indirectly test it by simplifying existing tests and utilising semantic selectors
https://testing-library.com/docs/queries/about#priority
For example, FormModifications.test.tsx may be simplified by using getByRole instead of getTextElementForField and getCheckBoxElementForField

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants