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.

Sign up for GitHub

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

Jump to bottom

GrafanaUI: Define tooltip or aria-label as required for IconButton #69699

Merged
merged 23 commits into from Jun 23, 2023
Merged

GrafanaUI: Define tooltip or aria-label as required for IconButton #69699

merged 23 commits into from Jun 23, 2023

Conversation

L-M-K-B
Copy link
Contributor

@L-M-K-B L-M-K-B commented Jun 7, 2023 •
edited

What is this feature?
In order to make sure every IconButton provides information about its purpose I now defined the tooltip or aria-label as required.

Why do we need this feature?
A11y reason: In case there is no aria-label set the tooltip is used to provide the required information for screen readers.
Consistency in GrafanaUI: Adding IconButton without proper information can be misleading and frustrating for users. This applies to all IconButtons.

Who is this feature for?
Everyone

Which issue(s) does this PR fix?:
Fixes #66346

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • The docs are updated

Release notice breaking change

For accessibility reasons tooltip or aria-label are now required properties for IconButton. In order to continue to use IconButton, you must ensure all IconButton components have a corresponding tooltip or aria-label text. The tooltip text will also be used as the aria-label if you didn't set one separately. In case you add an aria-label the IconButton will not show a tooltip.

@L-M-K-B L-M-K-B added add to changelog area/grafana/ui Issues that belong to components in the @grafana/ui library type/accessibility Accessibility problem / enhancement breaking change Relevant for changelog generation no-backport Skip backport of PR labels Jun 7, 2023
@L-M-K-B L-M-K-B added this to the 11.0.x milestone Jun 7, 2023
@L-M-K-B L-M-K-B mentioned this pull request Jun 7, 2023
Merged
@L-M-K-B L-M-K-B requested review from a team, mckn, joshhunt, eledobleefe and ashharrison90 and removed request for a team June 7, 2023 13:13
mckn
mckn approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

This is a good change and I like that we are improving a11y! In regards to plugins, I would say it is okay to proceed with this change as long as we don't write any code that relies on the properties being available since the grafana-packages are provided during runtime.

Plugin developers will get an error during build time when upgrading the grafana-packages which feels okay since it is really easy to fix.

Can you also add something in the migration guide from 10.0.x -> 10.1.x about this being a requirement and something that the plugin authors need to provide while upgrading the packages?

https://github.com/grafana/grafana/tree/main/docs/sources/developers/plugins/migration-guide

ckbedwell
ckbedwell reviewed Jun 8, 2023
View reviewed changes
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

In full support of this change!

I'd even be tempted to go one step further and make the visual tooltip always required and it doubles up as the aria-label if one isn't explicitly supplied but that might be too prescriptive...

packages/grafana-ui/src/components/IconButton/IconButton.tsx
/** Tooltip content to display on hover */
tooltip?: PopoverContent;
tooltip: PopoverContent;
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to discriminate further than this because you need to check if the Tooltip passed is a string or a component. If it is a string it can be used as the aria-label, if it is a component the aria-label will have to be explicitly set still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ckbedwell I changed the component quite a lot and after discussing what if the tooltip was not a string we decided to keep it as undefined for now. This use case doesn't occur too often so it should be fine.

We also discussed within the Frontend Platform team whether it makes sense to always require a tooltip and we identified some use cases in Grafana where a tooltip is not useful. For instance when IconButtons are used to expand context. They show a chevron and we think the icon is unambiguous in this case.

@L-M-K-B L-M-K-B force-pushed the laura/refactor/iconbutton-define-tooltip-as-required-v2 branch from b27e956 to 3975135 Compare June 12, 2023 12:05
@grafanabot grafanabot added datasource/Azure Azure Monitor Datasource datasource/Tempo labels Jun 14, 2023
@L-M-K-B L-M-K-B force-pushed the laura/refactor/iconbutton-define-tooltip-as-required-v2 branch from 5a9639a to a889fd8 Compare June 15, 2023 15:53
@github-actions
Copy link
Contributor

Backend code coverage report for PR #69699
No changes

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #69699

Plugin Main PR Difference
azuremonitor 86.11% 86.11% 0%

@L-M-K-B L-M-K-B marked this pull request as ready for review June 16, 2023 12:16
@L-M-K-B L-M-K-B requested review from a team as code owners June 16, 2023 12:16
@L-M-K-B L-M-K-B force-pushed the laura/refactor/iconbutton-define-tooltip-as-required-v2 branch from 1c4ffc5 to a2a0a40 Compare June 22, 2023 14:25
@L-M-K-B L-M-K-B merged commit d64b626 into main Jun 23, 2023
19 checks passed
@L-M-K-B L-M-K-B deleted the laura/refactor/iconbutton-define-tooltip-as-required-v2 branch June 23, 2023 15:10
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
@L-M-K-B @LudoVio
GrafanaUI: Define tooltip or aria-label as required for IconButton (#…
11e96cc 
…69699)

* refactor: modify interfaces to make tooltip or aria-label required

* refactor: change functionality around aria-label and tooltip

* refactor: change and add information in storybook documentation

* refactor: remove default from tooltip

* refactor: IconButton to make tooltip or aria-label required

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* feat: add migration guide for breaking change

* feat: add latest requirements to storybook docs

* refactor: separate iconbutton story with and without tooltip

* refactor: remove exported baseArgs

* refactor: clean up and restructure original story

* refactor: adjust styling

* refactor: enable control for tooltip

* refactor: clean up

* refactor: enable control for aria-label

* refactor: fix theme getting the wrong theme

* refactor: fix tests

* refactor: adjust story

* refactor: remove confusing story

* refactor: adjust controls for stories
harisrozajac pushed a commit that referenced this pull request Jun 29, 2023
@L-M-K-B @harisrozajac
GrafanaUI: Define tooltip or aria-label as required for IconButton (#…
b5179a3 
…69699)

* refactor: modify interfaces to make tooltip or aria-label required

* refactor: change functionality around aria-label and tooltip

* refactor: change and add information in storybook documentation

* refactor: remove default from tooltip

* refactor: IconButton to make tooltip or aria-label required

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* feat: add migration guide for breaking change

* feat: add latest requirements to storybook docs

* refactor: separate iconbutton story with and without tooltip

* refactor: remove exported baseArgs

* refactor: clean up and restructure original story

* refactor: adjust styling

* refactor: enable control for tooltip

* refactor: clean up

* refactor: enable control for aria-label

* refactor: fix theme getting the wrong theme

* refactor: fix tests

* refactor: adjust story

* refactor: remove confusing story

* refactor: adjust controls for stories
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
@L-M-K-B @harisrozajac
GrafanaUI: Define tooltip or aria-label as required for IconButton (#…
182b1ac 
…69699)

* refactor: modify interfaces to make tooltip or aria-label required

* refactor: change functionality around aria-label and tooltip

* refactor: change and add information in storybook documentation

* refactor: remove default from tooltip

* refactor: IconButton to make tooltip or aria-label required

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* refactor: Fix tests

* feat: add migration guide for breaking change

* feat: add latest requirements to storybook docs

* refactor: separate iconbutton story with and without tooltip

* refactor: remove exported baseArgs

* refactor: clean up and restructure original story

* refactor: adjust styling

* refactor: enable control for tooltip

* refactor: clean up

* refactor: enable control for aria-label

* refactor: fix theme getting the wrong theme

* refactor: fix tests

* refactor: adjust story

* refactor: remove confusing story

* refactor: adjust controls for stories
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mckn mckn mckn approved these changes

@tskarhed tskarhed tskarhed approved these changes

@joshhunt joshhunt Awaiting requested review from joshhunt joshhunt is a code owner automatically assigned from grafana/grafana-frontend-platform

@eledobleefe eledobleefe Awaiting requested review from eledobleefe eledobleefe is a code owner automatically assigned from grafana/grafana-frontend-platform

@ashharrison90 ashharrison90 Awaiting requested review from ashharrison90

@bossinc bossinc Awaiting requested review from bossinc bossinc is a code owner automatically assigned from grafana/partner-datasources

@alyssabull alyssabull Awaiting requested review from alyssabull alyssabull is a code owner automatically assigned from grafana/partner-datasources

@mgyongyosi mgyongyosi Awaiting requested review from mgyongyosi mgyongyosi was automatically assigned from grafana/grafana-authnz-team

@danielkenlee danielkenlee Awaiting requested review from danielkenlee danielkenlee was automatically assigned from grafana/grafana-authnz-team

@axelavargas axelavargas Awaiting requested review from axelavargas axelavargas is a code owner automatically assigned from grafana/dashboards-squad

@juanicabanas juanicabanas Awaiting requested review from juanicabanas juanicabanas is a code owner automatically assigned from grafana/dashboards-squad

@ckbedwell ckbedwell Awaiting requested review from ckbedwell

@Eve832 Eve832 Awaiting requested review from Eve832 Eve832 is a code owner

@josmperez josmperez Awaiting requested review from josmperez josmperez is a code owner

@wbrowne wbrowne Awaiting requested review from wbrowne wbrowne is a code owner automatically assigned from grafana/plugins-platform-backend

@andresmgot andresmgot Awaiting requested review from andresmgot andresmgot is a code owner automatically assigned from grafana/plugins-platform-backend

@JoaoSilvaGrafana JoaoSilvaGrafana Awaiting requested review from JoaoSilvaGrafana

Assignees
No one assigned
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library breaking change Relevant for changelog generation datasource/Azure Azure Monitor Datasource datasource/Tempo no-backport Skip backport of PR type/accessibility Accessibility problem / enhancement type/docs
Projects
None yet
Milestone
10.1.0
7 participants
@L-M-K-B @mckn @tskarhed @ckbedwell @JoaoSilvaGrafana @ricky-undeadcoders @grafanabot