-
Notifications
You must be signed in to change notification settings - Fork 312
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(form-core): improve validation feedback message for screen reader… #2471
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3217c94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments but I don't have strong opinions. Feel free to merge if my concerns are not to be concerned.
: nothing} | ||
</div> | ||
</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this break some style if consumers styled it assuming this is a block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can never break any tiny bit of code.
<div>
makes it a separate element the Screen reader can focus on. While it should be one sentence.
packages/ui/components/form-core/src/validate/LionValidationFeedback.js
Outdated
Show resolved
Hide resolved
…s to make it one sentence
71d673c
to
3217c94
Compare
|
||
el.feedbackData = [{ message: 'hello', type: 'info', validator: new AlwaysInvalid() }]; | ||
await el.updateComplete; | ||
|
||
expect(validationFeedbackType?.textContent?.trim()).to.equal('Info'); | ||
expect(validationFeedbackType?.textContent?.trim()).to.equal('Info,'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ,
(comma) better than :
(colon) ?
(In an older iteration of a design system we worked on we also had the sr-only "Error:" prefix)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried that one. Will see whats the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No difference. Either way is fine with me.
validationWarning: 'Предупреждение', | ||
validationSuccess: 'Успех', | ||
validationInfo: 'Информация', | ||
validationError: 'Грешка,', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's cleaner to add this inside .validation-feedback__type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first thought, @ryubro suggested to put it inside the translations :)
#2471 (comment)
…s to make it one sentence
What I did