fix(tabletoolbar): provide aria-label to CarbonTableToolbar #2860
Conversation
TheSharpieOne
requested review from
blechner,
bryancboyd,
cgirani,
davidicus,
herleraja,
jessieyan,
JoelArmendariz,
leetosc,
scottdickerson,
sstone2423,
stuckless and
tomklapiscak
as code owners
TheSharpieOne
force-pushed
the
fix/table-toolbar-aria-label
branch
from
c2b23d8 to
80ebbf1
Compare
|
✔️ Deploy Preview for ai-apps-pal-angular ready! 🔨 Explore the source changes: c2b23d8 🔍 Inspect the deploy log: https://app.netlify.com/sites/ai-apps-pal-angular/deploys/6148cee614456e0008760d26 😎 Browse the preview: https://deploy-preview-2860--ai-apps-pal-angular.netlify.app |
|
✔️ Deploy Preview for carbon-addons-iot-react ready! 🔨 Explore the source changes: c2b23d8 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-addons-iot-react/deploys/6148cee68ed47500075f37c3 😎 Browse the preview: https://deploy-preview-2860--carbon-addons-iot-react.netlify.app/ |
|
✔️ Deploy Preview for ai-apps-pal-angular ready! 🔨 Explore the source changes: a43ffb5 🔍 Inspect the deploy log: https://app.netlify.com/sites/ai-apps-pal-angular/deploys/614c8998e9b4020008f0ae87 😎 Browse the preview: https://deploy-preview-2860--ai-apps-pal-angular.netlify.app |
|
✔️ Deploy Preview for carbon-addons-iot-react ready! 🔨 Explore the source changes: a43ffb5 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-addons-iot-react/deploys/614cabb2f58ec300af3bc8e0 😎 Browse the preview: https://deploy-preview-2860--carbon-addons-iot-react.netlify.app |
| @@ -215,6 +216,7 @@ const TableToolbar = ({ | |||
| // TODO: remove deprecated 'testID' in v3 | |||
| data-testid={testID || testId} | |||
| className={classnames(`${iotPrefix}--table-toolbar`, className)} | |||
| aria-label={i18n.toolbarLabelAria || secondaryTitle ? `${secondaryTitle} Toolbar` : undefined} | |||
There was a problem hiding this comment.
Hi @TheSharpieOne. Thanks for the PR! Since this could change aria-lables that are out in the wild I think we should just use undefined if toolbarlabelAria is not present.
There was a problem hiding this comment.
Since aria-label could not be specified, the only value in the wild would be "data table toolbar", which is not a sane default. So it's not like the default truly is undefined. Default values should be useful so that users don't need to specify every prop every time (only needing to use props such as this for specials cases (e.g. i18n)).
Look at the changed snapshots and tell me this was not an improvement in a11y for the default case providing secondaryTitle.
I can mark this as a breaking change if you would like (carbon react doesn't seem to believe changes like this are considered breaking.)
There was a problem hiding this comment.
It is an improvement, and I'll let David answer your question, but it is also a known problem for some of our clients that changes to default labels and roles etc are not considered breaking since they are sometimes used in automated tests that fail after the change.
There was a problem hiding this comment.
rant
Yes, I believe it is a breaking change as well. It will definitely break snapshots (as seen by snapshots changing in the PR.) I just wasn't sure if this repo was more aligned with carbon react in it's definition of a breaking change since it's also within the carbon-design-system org.
We have had carbon react break us many times. Here are the most recent:
changes that prevent onChange callbacks from be called which break validation.
changes that add internal validation which affect component rendered output that cause visual changes.
/rant
I updated the commit message to indicate it is a breaking change. I included mitigation steps for users to preserve the current functionality to avoid changes.
TheSharpieOne
force-pushed
the
fix/table-toolbar-aria-label
branch
from
80ebbf1 to
831ff23
Compare
|
@TheSharpieOne I would be more than happy to include this in our next major release but that release date is still quite a ways off in the future. If this is something you will want to consume in the near term we should leave the defaults as undefined. If you are ok to wait for the next major release then we will want to open this against the v3 branch. |
TheSharpieOne
force-pushed
the
fix/table-toolbar-aria-label
branch
from
831ff23 to
d602170
Compare
Previously, aria-label could not be specified on TableToolbar, letting Carbon's TableToolbar default it to 'data table toolbar' This the label should be unqiue, but with multiple Tables with toolbars, it would only ever be 'data table toolbar' This change allows aria-label be be specified via the i18n prop as toolbarLabelAria
TheSharpieOne
force-pushed
the
fix/table-toolbar-aria-label
branch
from
d602170 to
a43ffb5
Compare
davidicus
added
status: ready to merge 🎉
and removed
status: waiting for author's response 💬
labels
Closes #2607
Summary
Change List (commits, features, bugs, etc)
toolbarLabelAriatoTable'si18nprop and forwards it toTableToolbarTableToolbarnow pulltoolbarLabelAriaout ofi18nto pass to the underlying carbon-componentAcceptance Test (how to verify the PR)
Regression Test (how to make sure this PR doesn't break old functionality)
Things to look for during review
iotorbxclass prefix is using the prefix variabledata-testidattribute. New test ids should have test written to ensure they are not changed or removed.