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): Clarification that developers should use ref with Input. Add a new example for Storybook. #1526

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

nikitaorliak-cengage
Copy link
Collaborator

Issue: #1124

What I did

Add clarification that developers should use ref with Input. Add a new example for Storybook.

Screenshots:

Checklist

  • changeset has been added
  • Pull request description is descriptive
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

How to test

Open Input in the docs -> Open Error message -> Leave the input blank and click Submit -> Check that the input has focus.

@nikitaorliak-cengage nikitaorliak-cengage added the ready for review Pull requests that are ready for developer review label Nov 4, 2024
@nikitaorliak-cengage nikitaorliak-cengage self-assigned this Nov 4, 2024
Copy link

changeset-bot bot commented Nov 4, 2024

🦋 Changeset detected

Latest commit: 9383400

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

This PR includes changesets to release 1 package
Name Type
react-magma-docs 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

Copy link
Contributor

github-actions bot commented Nov 4, 2024

Copy link
Contributor

github-actions bot commented Nov 4, 2024

@silvalaura
Copy link
Collaborator

Scheduled a meeting to discuss this as a team, so lets put this on hold for now.

@@ -170,6 +170,8 @@ export function Example() {
If an input has an `errorMessage`, the input will be styled to highlight it's error state and the error message will appear below the input field.
If an error message is present, it will replace the helper text. Can be a node or a string.

For best practices, use a `ref` on the input to enhance usability when handling an `errorMessage`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can move this sentence to go right after While React Magma provides the error styling, it is up to the consumer app to handle the validation and we can elaborate a little more

It would read something like:

While React Magma provides the error styling, it is up to the consumer app to handle the validation. We recommend using a ref on the input for accessibility.

For short forms with an error, clicking submit should bring the focus back to the input with an error. For long forms, we recommend using an alert to combine the errors and focus should be moved to the alert. See example in Form.

@silvalaura silvalaura removed the ready for review Pull requests that are ready for developer review label Nov 21, 2024
Copy link
Contributor

Copy link
Contributor

Nikita Orliak added 2 commits November 22, 2024 01:28
Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

website/react-magma-docs/src/pages/api/input.mdx Outdated Show resolved Hide resolved
header="Form Heading"
description="Some Form Description"
errorMessage="Some Form Error"
errorMessage={errorMessage}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, the only thing is that the alert does not get focus / the message is not announced with a screenreader

Copy link
Collaborator

@silvalaura silvalaura Dec 18, 2024

Choose a reason for hiding this comment

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

This has not been addressed yet: https://jam.dev/c/7e5f61b7-cf01-4511-acdc-409d1b8760e0

I think I expect the focus to move to the alert so that the error message can be read. Maybe it needs to be assertive

Copy link
Contributor

Copy link
Contributor

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Copy link
Collaborator

@silvalaura silvalaura left a comment

Choose a reason for hiding this comment

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

Merge conflicts also need to be addressed

Copy link
Contributor

Copy link
Contributor

Copy link
Collaborator

@silvalaura silvalaura left a comment

Choose a reason for hiding this comment

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

Just missing addressing this, since this is what we describe in the docs:
#1526 (comment)

@silvalaura silvalaura added the ready for review Pull requests that are ready for developer review label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull requests that are ready for developer review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input: Error message not read by screenreader without helperMessage
2 participants