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

Variety of quick a11y fixes #46569

Merged
merged 6 commits into from
Oct 3, 2019
Merged

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Sep 25, 2019

(## Summary

A slew of accessibility fixes for a wide range of issues, mostly around document structure. Focusing on things that are easily detectable by automated tools (which we keep getting flagged for in audits) and things that are easy to fix.

Not all solutions are perfect but they're improvements on what we have today.

Addresses some pages of #37539 (Home, Discover, Dashboard, Visualize). Probably more can be done but this is a good start.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@myasonik myasonik requested a review from a team September 25, 2019 14:38
@timroes timroes self-requested a review September 25, 2019 14:41
@elasticmachine
Copy link
Contributor

💔 Build Failed

@myasonik myasonik requested a review from a team as a code owner September 26, 2019 11:13
@sulemanof
Copy link
Contributor

Hi @myasonik !
I would suggest to revert all changes in src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/heatmap_options.html because I have an opened PR #45766 with EUI-fying of this stuff. I've added you as a reviewer. I also tried to follow the general structure you've done in your PR and replaced all the headings to h3 tag.
And I'd also like you to explain the necessity of such a replacement!
Thanks in advance.

@myasonik
Copy link
Contributor Author

@sulemanof Previously I suggested using an h2 because of the existing heading structure. I think the sidebar title was set to be the h1 on the page which would make the headings inside of the sidebar h2s.

Now, I've gone and added a proper h1 to the page (though it's visually hidden) that describes the whole page. The sidebar header is now an h2 which makes all of the sections inside of the sidebar h3 headers.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Overall looks really good, got a couple of minor code style comments.

aria-label={this.props.intl.formatMessage(
{
id: 'data.query.queryBar.searchInputAriaLabel',
defaultMessage: 'Start typing to search and filter the {pageType} page',
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the default message of a string, you should remove all existing translations for this string, so it will be retranslated by our translators again. Just search for that id and remove it from all internationalization files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like all of them were already removed... Am I missing anything?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM, looked at a couple of changes and checked it didn't break styling.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM

@kertal kertal self-requested a review October 2, 2019 13:24
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Change in Discover LGTM 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@myasonik myasonik merged commit ea18949 into elastic:master Oct 3, 2019
@myasonik myasonik deleted the axe-a11y-fixes branch October 3, 2019 09:30
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 3, 2019
…ysens/kibana into console-fix-menu-actions-alignment

* 'console-fix-menu-actions-alignment' of github.com:jloleysens/kibana: (59 commits)
  [Canvas] i18n for page manager, page preview, and shape preview (elastic#46865)
  [SIEM] hide siem data on empty string or _all (elastic#47166)
  [Uptime] Shim UI exports for new platform (elastic#44722)
  [Monitoring] Metricbeat Migration Wizard Tests (elastic#47139)
  [Logs UI] Support partitioned log rate results in API (elastic#46751)
  [APM] Remove beta badge from transaction breakdown (elastic#47163)
  test: 💍 convert Interpreter font function tests to Jest (elastic#47193)
  visualizations 👉 NP-ready (elastic#47142)
  [Uptime] Update pings chart colors (elastic#46780)
  Add scroll into view to avoid tooltip overlap (elastic#47106)
  Variety of quick a11y fixes (elastic#46569)
  [Graph] Empty workspace overlay (elastic#45547)
  [ML] Converts index and saved search selection to React (elastic#47117)
  [Uptime] Change default status filter in ping list to all on monitor page  (elastic#47108)
  refactor: 💡 convert Interpreter .js -> .ts (elastic#44545)
  Add TypeScript rules to STYLEGUIDE [skip ci] (elastic#47125)
  chore(NA): fix logic behind cleaning x-pack node modules on build (elastic#47091)
  [SIEM] Update Settings Text (elastic#47147)
  Add KQL functionality in the find function of the saved objects (elastic#41136)
  [Maps] Add 'InjectedData' class and revise so File Upload Features are assigned to new InjectedData instances (elastic#46381)
  ...
myasonik pushed a commit that referenced this pull request Oct 3, 2019
Focusing on heading structure and page layout for Home, Discover, Dashboard, and Visualize.

This is progress on #37539
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.

9 participants