Skip to content

Conversation

@PhilippBaranovskiy
Copy link
Contributor

@PhilippBaranovskiy PhilippBaranovskiy commented Jun 14, 2019

Fixes #1697

Summary

To allow setting aria-label attribute override default Select this row value I've put a conditional assigning value.
Now if selection.selectionMessage is provided, then aria-label value would be equal to that function's output.
Otherwise, the default value is applied.

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

Copy link

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM. This will allow elastic/kibana#28390 to be addressed for example.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

The changes LGTM. I wonder if we should add a quick sentence about the importance of adding the ariaLabel prop in the docs here:

@PhilippBaranovskiy
Copy link
Contributor Author

The changes LGTM. I wonder if we should add a quick sentence about the importance of adding the ariaLabel prop in the docs here:

Agree, I will drop a line there.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Pulling the value from item.ariaLabel requires the dataset to be modified for the table, and this is something we have seen engineers push back on. Instead, we prefer having lookup functions that are passed the item and return the desired value. As an example, look at the table's use of selection.selectableMessage.

Which leads to another thought, it appears we're already using selection.selectableMessage for this exact case, but it isn't being added to aria-label. Should we re-use that value? Related: #2021

@PhilippBaranovskiy
Copy link
Contributor Author

PhilippBaranovskiy commented Jun 18, 2019

@chandlerprall, such a brilliant point, thanks for that!
I've updated the PR.
Could you have a look and let me know whether I did get that right?

@chandlerprall
Copy link
Contributor

@cchaos what are you thoughts on re-using the existing title value on the selection checkbox for aria-label?

@PhilippBaranovskiy
Copy link
Contributor Author

jenkins, test this

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

what are you thoughts on re-using the existing title value on the selection checkbox for aria-label?

I am fine with that. They are typically the same. I can't see a reason why they should ever be different.

@chandlerprall
Copy link
Contributor

@rockfield Let's remove the new method re-use the existing title variable as the aria-label

@PhilippBaranovskiy
Copy link
Contributor Author

@chandlerprall @cchaos
I've reverted back the docs, passing getAriaLabel function and multiple translations.
Now, as Chandler suggested, I'm just assigning the title value to aria-label.

Please, could you have a look and confirm?
Now it's just a couple of lines PR.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One last change, sorry!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks @rockfield !

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.

[Accessibility] Allow setting of the aria-label of table row selection checkboxes

4 participants