-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
💔 Build Failed
|
e35ccc4
to
6cdd1bc
Compare
💚 Build Succeeded
|
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! |
This direction looks good to me 👍 |
e8086a8
to
5b65004
Compare
There was a problem hiding this 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
Thanks, @igoristic. I've updated the PR to make capitalization more consistent. |
There was a problem hiding this 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
x-pack/legacy/plugins/monitoring/public/components/no_data/no_data.js
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/monitoring/public/components/elasticsearch/node/node.js
Show resolved
Hide resolved
4aa7e55
to
130d1d8
Compare
@chrisronline I wrapped up your remaining concerns this morning. Could you please take another look? Thanks! |
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
💔 Build Failed
History
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
3 similar comments
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* [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>
…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>
I also had conflicts because of this PR in #115826 |
Add h1 headings to components in Stack Monitoring.
Refs #52275