-
Notifications
You must be signed in to change notification settings - Fork 276
feat(ui5-table-selection-multi): headerSelector property added #11711
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
base: main
Are you sure you want to change the base?
Conversation
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.
Forgot to submit my comments, sorry for that
name={ClearAll} | ||
mode={IconMode.Decorative} | ||
showTooltip={this._hasSelectedRows} | ||
accessibleName={this._hasSelectedRows ? this._i18nDeselectAllRows : undefined} |
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 feel like this should always have an accessible name, even if there are no rows selected. Otherwise, I could imagine people complaining, that focusing the cell won't announce anything, if no rows are selected.
Maybe in those cases, where no rows are selected, an active state can be announced?
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 a topic for Oliver?
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 will check with Oliver maybe we should really change the aria-label of the cell depending on the state (line 15)
<Icon | ||
name={ClearAll} | ||
mode={IconMode.Decorative} | ||
showTooltip={this._hasSelectedRows} |
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.
Goes hand in hand with the below comment. If no rows are selected, there will be no tooltip (as it relies on accessibleName).
Not sure if this should be how it is
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.
The mode is Decorative so the role is presentation.
If no rows are selected, then the Icon is NonInteractive like it is not there.
The cell has its own aria-label "Row Selector"
- provides an enumeration property to switch in the multi-select case between a select all and a clear all option (in future further variants might be added). Fixes: #5873
- provides an enumeration property to switch in the multi-select case between a select all and a clear all option (in future further variants might be added). Fixes: #5873
- provides an enumeration property to switch in the multi-select case between a select all and a clear all option (in future further variants might be added). - CSS intentation fix Fixes: #5873
a132078
to
04600df
Compare
*/ | ||
enum TableSelectionMultiHeaderSelector { | ||
/** | ||
* Renders a checkbox in the table header row to toggle the selection of all rows. |
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.
* Renders a checkbox in the table header row to toggle the selection of all rows. | |
* Renders a checkbox in the table header row that toggles the selection of all rows. |
SelectAll = "SelectAll", | ||
|
||
/** | ||
* Renders an icon in the table header row to remove the selection of all rows. |
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.
* Renders an icon in the table header row to remove the selection of all rows. | |
* Renders an icon in the table header row that removes the selection of all rows. |
(in future further variants might be added).
Fixes: #5873