Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-15993 - Making autocomplete a11y compatible #3048

Merged
merged 5 commits into from
Jul 5, 2019
Merged

MM-15993 - Making autocomplete a11y compatible #3048

merged 5 commits into from
Jul 5, 2019

Conversation

asaadmahmood
Copy link
Contributor

Summary

MM-15993 - Making autocomplete a11y compatible

Ticket Link

https://mattermost.atlassian.net/browse/MM-15993

@asaadmahmood asaadmahmood added this to the v5.14.0 milestone Jul 3, 2019
@asaadmahmood asaadmahmood added 2: Dev Review Requires review by a core commiter Do Not Merge Should not be merged until this label is removed labels Jul 3, 2019
@asaadmahmood
Copy link
Contributor Author

@deanwhillier This works.
@hmhealey See if this is okay, or we should give the suggestionReadOut div in search_suggestion_list.jsx a ref and try to pass it to the child component. (Not sure if the child component can fetch ref on a parent component, but maybe it can using forwardRef).

hmhealey
hmhealey suggested changes Jul 3, 2019
View reviewed changes
components/suggestion/search_channel_provider.jsx Outdated
@@ -26,12 +26,31 @@ function itemToName(item) {
}

class SearchChannelSuggestion extends Suggestion {
componentDidMount() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of implementing this in both the SearchChannelSuggestion and SearchUserSuggestion components, couldn't you just make the SearchSuggestionList handle this itself? It already knows what value has been suggested, and we won't have to use IDs to do this then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @hmhealey

hmhealey
hmhealey suggested changes Jul 4, 2019
View reviewed changes
components/suggestion/search_suggestion_list.jsx Outdated Show resolved Hide resolved
components/suggestion/search_suggestion_list.jsx Outdated Show resolved Hide resolved
@asaadmahmood
Copy link
Contributor Author

Done @hmhealey

deanwhillier
deanwhillier approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@deanwhillier deanwhillier left a comment

Choose a reason for hiding this comment

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

LGTM.

Note, when the a11y-controller PR is merged, keyboard-focus class names will need to be changed to a11y--focus.

@asaadmahmood asaadmahmood removed the Do Not Merge Should not be merged until this label is removed label Jul 4, 2019
@asaadmahmood
Copy link
Contributor Author

Cool. @deanwhillier @hmhealey We can get it in if it all looks good.

@asaadmahmood asaadmahmood merged commit 8c5986e into mattermost:master Jul 5, 2019
@asaadmahmood asaadmahmood deleted the MM-15993 branch July 5, 2019 10:04
@asaadmahmood asaadmahmood mentioned this pull request Jul 5, 2019
Merged
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jul 8, 2019
@amyblais amyblais added the Changelog/Done Required changelog entry has been written label Jul 16, 2019
@amyblais amyblais added the Docs/Needed Requires documentation label Jul 16, 2019
@lindy65 lindy65 added Tests/Done Release tests have been written and removed 4: Reviews Complete All reviewers have approved the pull request labels Jul 19, 2019
@esethna esethna added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@deanwhillier deanwhillier deanwhillier approved these changes

@hmhealey hmhealey hmhealey approved these changes

Assignees

@esethna esethna

Labels
Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.14.0
6 participants
@asaadmahmood @deanwhillier @hmhealey @lindy65 @esethna @amyblais