Skip to content
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(data-table): use unselectAll translation key if selection checkbox is indeterminate #4570

Merged
merged 2 commits into from
Nov 6, 2019
Merged

fix(data-table): use unselectAll translation key if selection checkbox is indeterminate #4570

merged 2 commits into from
Nov 6, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Nov 5, 2019

Closes #4556

Changelog

Changed

  • check for indeterminate checkbox (as well as checked to determinate if translationKeys.unselectAll is used

Testing / Reviewing

Open the carbon-components-react deploy and follow the steps in #4556 -- directly select only a few rows, and then check the aria-label of the indeterminate icon. It should say "unselect all rows" instead of "select all rows"

@jendowns jendowns requested a review from a team as a code owner November 5, 2019 17:31
@ghost ghost requested review from abbeyhrt and jnm2377 November 5, 2019 17:31
@jendowns jendowns changed the title fix(data-table): use correct aria-label for indeterminate unselect all fix(data-table): use unselectAll translation key if selection checkbox is indeterminate Nov 5, 2019
@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for the-carbon-components ready!

Built with commit f069b6f

https://deploy-preview-4570--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-components-react ready!

Built with commit f069b6f

https://deploy-preview-4570--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for carbon-elements ready!

Built with commit f069b6f

https://deploy-preview-4570--carbon-elements.netlify.com

@carmacleod
Copy link
Contributor

Tested react version. Looks good! Thanks, @jendowns!

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

? translationKeys.unselectAll
: translationKeys.selectAll;
const translationKey =
checked || indeterminate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
checked || indeterminate
(checked || indeterminate)

Does prettier end up taking away the parenthesis here? There seems to be some syntax ambiguity that adding these in might help with. Otherwise, seems great!

Copy link
Member

Choose a reason for hiding this comment

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

yeah prettier will remove parentheses there. by default it is evaluated as (checked || indeterminate) ? ... : ... rather than checked || (indeterminate ? ... : ...)

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me

@jnm2377 jnm2377 merged commit 10f2fae into carbon-design-system:master Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataTable with selection aria-label text should say "Unselect all rows" when aria-checked="mixed"
6 participants