-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Kashkovsky intl a11y, attempt 2 #4161
Conversation
…e messages contains wrong statement
… the one supplied in props
💥 No ChangesetLatest commit: 24d1e60 Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 24d1e60:
|
packages/react-select/src/Select.js
Outdated
@@ -73,8 +75,41 @@ type FormatOptionLabelMeta = { | |||
inputValue: string, | |||
selectValue: ValueType, | |||
}; | |||
export type AccessibilityProp = { |
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.
What do you think about moving these to a separate file?
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 think you are right! I'll try to find a good place for them.
@@ -1815,10 +1868,14 @@ export default class Select extends Component<Props, State> { | |||
renderLiveRegion() { |
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.
It might be good to abstract this to a separate component at this time. What do you think?
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.
Thanks for your feedback!
I am not sure. Looking at other render helpers, renderLiveRegion
seems to line up pretty well? On the other hand one could argue for a decoupling exercise of the whole Select
class, being ~1600 lines tall. But perhaps that work should be logged in a separate work package. What are you thoughts?
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 agree! You are following the convent. Also agree that this is 1600 lines. So maybe another PR would be good to refactor some of the render* function into separate components.
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.
Hi guys, the example looks great! Btw Is anyone working on merging this PR? Do you guys need any help? It seems @TheHollidayInn has done all the job. |
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.
@TheHollidayInn @radegran Would you mind taking a look at the comments regarding the propName? accessibility
as a prop name simply would not be descriptive enough. Recommendation would be ariaLiveConfig
or something of a similar nature.
@@ -75,6 +79,8 @@ type FormatOptionLabelMeta = { | |||
}; | |||
|
|||
export type Props = { | |||
/* Custom ARIA message functions */ | |||
accessibility?: AccessibilityProp, |
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 this is a bit of semantics, but I believe accessibility
is a bit vague for a prop.
Would you be willing to consider something more along the lines of ariaLiveConfig
? I think it better encapsulates the functionality.
Additionally, this would provide more clarity and avoids confusion with perhaps other things aria/accessibility related.
OverviewThis proposed PR adds the ability for users to configure the ariaLive messaging configuration to allow custom messages. ProblemThis addresses two gaps:
SolutionThe internal Related Issues solved by this#3352 Aria messages cannot be localized |
@kylemh regarding your comments from here, perhaps you were mistaking this with another PR because of the I also agree that having an "accessibility catch-all prop" that includes all things aria would not be a good idea. |
💯 thank you for helping me read correctly The name change would help for sure. |
Note to future self, pull this PR with documentation PR and rename prop name |
Please note that there has been no response to requests for updates. I have began creating a new PR inspired by this and hope to address several other gaps in the aria live messaging. I will close this once I have a submission ready. |
Greetings @radegran , Thank you for your efforts resurrecting this PR. Your efforts are much appreciated. There were some issues with merging and code review suggestions that were not addressed so I have taken these efforts as a starting point to create #4390 . I will be closing this issue so we can continue on with attempt 3. |
@ebonow, I have not been keeping track of this PR for some time. Glad to see you moving forward with it, best of luck! |
Fixed conflicts and added test for PR #3550 that has come to a halt.