fix(material/sort): add description input for sort-header #23633
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jelbourn
added
P2
google-cla
bot
added
the
cla: yes
crisbeto
reviewed
Sep 24, 2021
| @@ -176,6 +199,9 @@ export class MatSortHeader extends _MatSortHeaderBase | |||
| {toState: this._isSorted() ? 'active' : this._arrowDirection}); | |||
|
|
|||
| this._sort.register(this); | |||
|
|
|||
| this._sortButton = this._elementRef.nativeElement.querySelector('[role="button"]')!; | |||
There was a problem hiding this comment.
Should we use a ViewChild query to get the button instead of querying directly?
There was a problem hiding this comment.
IMO I don't see the point of using @ViewChild to query for native DOM elements (vs. directives or injectables)
|
|
||
| // If _sortButton is undefined, the component hasn't been initialized yet so there's | ||
| // nothing to update in the DOM. | ||
| if (this._sortButton) { |
There was a problem hiding this comment.
Do we also need to check that the description is defined/isn't an empty string here?
There was a problem hiding this comment.
AriaDescriber already checks the presence of the message internally
jelbourn
force-pushed
the
sort-a11y
branch
2 times, most recently
from
f2e49a2 to
3bb13ed
Compare
crisbeto
approved these changes
Sep 24, 2021
zarend
approved these changes
Sep 24, 2021
src/components-examples/material/table/table-sorting/table-sorting-example.ts
Show resolved
Hide resolved
jelbourn
added
target: patch
Adds a description input for mat-sort-header so that developers can provide an accessible description (using AriaDescirber under the hood). Additionally update the accessibility section for the sort header's documentation with guidance on providing an accessible experience. I decided to use this approach instead of expanding `MatSortHeaderIntl` because the message developers would want to set here depends on several bits of information, including: * Whether the column is currently sorted * The sort direction * Whether users can clear sorting (configured on `MatSort`) * The name of the column (not the ID) Accounting for all of these factors would have made the intl formatting too complicated. This does have the negative consequence of needing to set a description for every header. However, users can add a custom directive to set the description in a consistent way if they have an application with many tables.
zarend
pushed a commit
that referenced
this pull request
Oct 13, 2021
Adds a description input for mat-sort-header so that developers can provide an accessible description (using AriaDescirber under the hood). Additionally update the accessibility section for the sort header's documentation with guidance on providing an accessible experience. I decided to use this approach instead of expanding `MatSortHeaderIntl` because the message developers would want to set here depends on several bits of information, including: * Whether the column is currently sorted * The sort direction * Whether users can clear sorting (configured on `MatSort`) * The name of the column (not the ID) Accounting for all of these factors would have made the intl formatting too complicated. This does have the negative consequence of needing to set a description for every header. However, users can add a custom directive to set the description in a consistent way if they have an application with many tables. (cherry picked from commit ecd54f4)
zarend
pushed a commit
that referenced
this pull request
Oct 13, 2021
Adds a description input for mat-sort-header so that developers can provide an accessible description (using AriaDescirber under the hood). Additionally update the accessibility section for the sort header's documentation with guidance on providing an accessible experience. I decided to use this approach instead of expanding `MatSortHeaderIntl` because the message developers would want to set here depends on several bits of information, including: * Whether the column is currently sorted * The sort direction * Whether users can clear sorting (configured on `MatSort`) * The name of the column (not the ID) Accounting for all of these factors would have made the intl formatting too complicated. This does have the negative consequence of needing to set a description for every header. However, users can add a custom directive to set the description in a consistent way if they have an application with many tables. (cherry picked from commit ecd54f4)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a description input for mat-sort-header so that developers can provide an accessible description (using AriaDescirber under the hood). Additionally update the accessibility section for the sort header's documentation with guidance on providing an accessible experience.
I decided to use this approach instead of expanding
MatSortHeaderIntlbecause the message developers would want to set here depends on several bits of information, including:MatSort)Accounting for all of these factors would have made the intl formatting too complicated.
This does have the negative consequence of needing to set a description for every header. However, users can add a custom directive to set the description in a consistent way if they have an application with many tables.
cc @zarend @amysorto