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

[Monitoring] h1 elements for accessibility #52276

Merged
merged 29 commits into from
Jan 9, 2020

Conversation

cachedout
Copy link
Contributor

Add h1 headings to components in Stack Monitoring.

Refs #52275

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cachedout
Copy link
Contributor Author

Hi @chrisronline I know you were looking this over the others day. Any thoughts on this approach generally? I can finish up the rest pretty quickly but just wanted a 👍 that the general approach looks good. Thanks!

@chrisronline
Copy link
Contributor

This direction looks good to me 👍

@cachedout cachedout marked this pull request as ready for review December 13, 2019 14:55
@cachedout cachedout changed the title [Monitoring] [WIP] h1 elements for accessibility [Monitoring] h1 elements for accessibility Dec 13, 2019
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

@cachedout Looks good 👍 One thing I noticed is that ... Overview is not always capitalized. Should we maybe keep it consistent and title case it if it's a heading?

I tested some of it with chrome accessibility and it appears to be working as expected

@cachedout
Copy link
Contributor Author

Should we maybe keep it consistent and title case it if it's a heading?

Thanks, @igoristic. I've updated the PR to make capitalization more consistent.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Nice job overall! I left a few questions/comments

@cachedout
Copy link
Contributor Author

@chrisronline I wrapped up your remaining concerns this morning. Could you please take another look? Thanks!

@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@elastic elastic deleted a comment from elasticmachine Dec 27, 2019
@cachedout
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@cachedout
Copy link
Contributor Author

@elasticmachine merge upstream

3 similar comments
@cachedout
Copy link
Contributor Author

@elasticmachine merge upstream

@cachedout
Copy link
Contributor Author

@elasticmachine merge upstream

@cachedout
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cachedout
Copy link
Contributor Author

@chrisronline Given that we couldn't find a straightforward to address your review comment, I re-requested a review from you to see if you're OK with merging this as-is.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

@cachedout cachedout merged commit 459cad5 into elastic:master Jan 9, 2020
@cachedout cachedout deleted the issue_37539 branch January 9, 2020 15:31
@matschaffer
Copy link
Contributor

matschaffer commented Oct 15, 2021

Just a heads up that I think this is missing a 7.x backport. I noticed when backporting #115013 (#115140) which had some conflicts on lines introduced in this PR.

Lemme know if anyone thinks we should open a backport for it at this stage or what.

matschaffer added a commit to matschaffer/kibana that referenced this pull request Oct 15, 2021
matschaffer added a commit that referenced this pull request Oct 15, 2021
* [Stack Monitoring] Fix monitoring func test (#115013)

* add noDataContainer test-subj

* remove unnecessary test step

* Fix test subjects for overview page

* remove unused service

* update NoData component snapshots

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Update snaps to include items not backported from #52276

Co-authored-by: Kevin Lacabane <kevin.lacabane@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
estermv added a commit to estermv/kibana that referenced this pull request Oct 21, 2021
estermv added a commit that referenced this pull request Oct 21, 2021
…gular components (#115593) (#115826)

* [Stack monitoring] Remove getAngularInjector and duplicated angular components (#115593)

* remove getAngularInjector and old angular components

* Remove suffix from CcrShardReact component

* Remove suffix from ElasticsearchOverviewReact component

* Remove suffix from indexReact component

* Remove suffix from NodeReact component

* Remove suffix from ShardActivityReact

* Remove suffix from ShardAllocationReact component and its childs

* Fix import

* fix translations

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/monitoring/public/components/elasticsearch/node/node.js

* Fix missing code not backported from #52276

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@estermv
Copy link
Contributor

estermv commented Oct 25, 2021

I also had conflicts because of this PR in #115826

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.

7 participants