Skip to content

[Lens] (Accessibility) Added focus state and accessible name to suggestions #84653

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
Merged
merged 4 commits into from
Dec 4, 2020
Merged

[Lens] (Accessibility) Added focus state and accessible name to suggestions #84653

merged 4 commits into from
Dec 4, 2020

Conversation

@mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Dec 1, 2020 •
edited
Loading

Fixes #83602

  • Focus styles and hover styles demo:
    image

(gif didn't pick up the color difference between light background of selected vis but it's there)
suggestions

  • Changed name of current suggestion from 'current' to 'current visualization'
  • added aria-current={true} for current vis
  • added aria-label with suggestion name

@mbondyra mbondyra added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Dec 1, 2020
@mbondyra mbondyra force-pushed the lens/accessibility-suggestions-name-focus branch from a4874d6 to 07d8b6b Compare December 1, 2020 16:13
@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Dec 1, 2020

@mbondyra: FYI, left a comment on the related issue with my suggestions.

@mbondyra mbondyra closed this Dec 3, 2020
@mbondyra mbondyra force-pushed the lens/accessibility-suggestions-name-focus branch from 07d8b6b to 9041ea5 Compare December 3, 2020 09:52
@mbondyra mbondyra reopened this Dec 3, 2020
@elastic elastic deleted a comment from kibanamachine Dec 3, 2020
@mbondyra mbondyra marked this pull request as ready for review December 3, 2020 10:07
@mbondyra mbondyra requested a review from a team December 3, 2020 10:07
@mbondyra mbondyra requested a review from a team as a code owner December 3, 2020 10:07
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 3, 2020

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

flash1293 commented Dec 3, 2020

Something happened to the embedded expression renderers - the padding is much larger:

old:
Screenshot 2020-12-03 at 14 30 58

new:
Screenshot 2020-12-03 at 14 31 03

Did you do this on purpose? I think it's also causing "Current visualization" to be cut of.

myasonik
myasonik approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

The a11y looks good! 🎉

Adding @MichaelMarcialis to take a look at the styles

MichaelMarcialis
MichaelMarcialis approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

This looks great to me. Left two small last second suggestions. Otherwise, this should be good to go. Approving now so I don't hold you up any further.

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.scss
}
}

.lnsSuggestionPanel__button-isSelected {
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please add a border-color: $euiColorMediumShade; within this selector's (.lnsSuggestionPanel__button-isSelected) styles. I think this added contrast will help the selected button stand out just a bit more.

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.scss
}
}

.lnsSuggestionPanel__button-isSelected {
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please also add the following nested styles within this selector's (.lnsSuggestionPanel__button-isSelected) styles. Doing so will prevent the selected button from transitioning up 2px on hover (which feels odd on the selected button).

&:hover {
  transform: none !important; // sass-lint:disable-line no-important
}

flash1293
flash1293 approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Chrome and Firefox and works fine, LGTM, just left a nit

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx
icon:
visualizationMap[currentVisualizationId].getDescription(currentVisualizationState)
.icon || 'empty',
title: i18n.translate('xpack.lens.suggestions.currentVisLabel', {
Copy link
Contributor

@flash1293 flash1293 Dec 4, 2020

Choose a reason for hiding this comment

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

nit: Please remove the translations for this phrase

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 4, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 995.5KB 997.6KB +2.0KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra mbondyra merged commit 2cf4e72 into elastic:master Dec 4, 2020
@mbondyra mbondyra deleted the lens/accessibility-suggestions-name-focus branch December 4, 2020 18:37
mbondyra added a commit to mbondyra/kibana that referenced this pull request Dec 4, 2020
@mbondyra
[Lens] (Accessibility) Added focus state and accessible name to sugge…
b6e42b0 
…stions (elastic#84653)

* [Lens] (Accessibility) Added focus state and accessible name to suggestions

* Apply suggestions from code review

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

padding oops

* cr
@mbondyra mbondyra mentioned this pull request Dec 4, 2020
Merged
mbondyra added a commit that referenced this pull request Dec 5, 2020
@mbondyra
[Lens] (Accessibility) Added focus state and accessible name to sugge…
78a7feb 
…stions (#84653) (#85054)

* [Lens] (Accessibility) Added focus state and accessible name to suggestions

* Apply suggestions from code review

* Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

padding oops

* cr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 approved these changes

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

+1 more reviewer

@myasonik myasonik myasonik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] (Accessibility) Suggestions have no focus state and no accessible name

6 participants

@mbondyra @MichaelMarcialis @elasticmachine @flash1293 @kibanamachine @myasonik