[ML] (Accessibility) Fix page heading structure #56741
Conversation
peteharverson
requested review from
darnautov,
alvarezmelissa87 and
jgowdyelastic
|
Pinging @elastic/ml-ui (:ml) |
| .influencer-title { | ||
| font-size: $euiFontSizeM - 2px; | ||
| font-weight: 600; | ||
| line-height: 24px; | ||
| } |
There was a problem hiding this comment.
Is it possible at all to use one of the pre-existing title sizing without overrides? We've created the title scale with the intention that we'd have consistency throughout all the apps in Kibana. I'm sure one of these sizes should fit this use case. http://eui.elastic.co/#/display/title
There was a problem hiding this comment.
@cchaos good spot - I have managed to remove this override, plus another one I was using on the data visualizer cards in 6f1de6b.
I have decided to leave some panel-title overrides for headings on the Anomaly Explorer and Single Metric Viewer for now, to retain the current size (18px) and color. Note @mdefazio is currently looking at some options for headings and controls on these two pages, so we may be able to clean these up in a future PR.
| @@ -61,6 +61,8 @@ | |||
| } | |||
|
|
|||
| .panel-title { | |||
| font-size: $euiFontSizeM; | |||
There was a problem hiding this comment.
nit: might be better to use EuiTitle size attribute instead
There was a problem hiding this comment.
Going to stick with euiFontSizeM here - it is 18px, and works well with the existing content in the page. For EuiTitle the size options are only giving me a choice of 16px or 20px.
| font-size: $euiFontSizeM; | ||
| font-weight: 400; | ||
| color: $euiColorDarkShade; |
There was a problem hiding this comment.
Already spotted .panel-title class with similar styles 👀
There was a problem hiding this comment.
As above, going to stick with euiFontSizeM here as it retains the 18px font size used previously.
| @@ -1206,157 +1206,167 @@ export class TimeSeriesExplorer extends React.Component { | |||
| jobs.length > 0 && | |||
| (fullRefresh === false || loading === false) && | |||
| hasResults === true && ( | |||
| <div> | |||
| <div className="results-container"> | |||
There was a problem hiding this comment.
This change now contradicts the the comment below (... inside this **plain** wrapping element ...) can you check this doesn't break tooltip positioning? Depending on the outcome we should fix the div or adapt the comment.
There was a problem hiding this comment.
Good spot. I have added the extra plain div back in (one with 0px left/right padding) in 5efc092. I also edited the comment used in explorer.js to clarify the role of the container div for the ChartTooltip.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* [ML] (Accessibility) Fix page heading structure * [ML] Fix file datavisualizer SCSS import * [ML] Switch to EuiTitle for influencers list and data viz cards * [ML] Fix positioning of chart tooltip in Single Metric Viewer
Summary
Part of #37539 and #52278, fixes landmark and heading structure for pages in the ML plugin.
Changes made ensure pages meet following WCAG criteria:
<h1><h1>-<h2>-<h3>Pages were checked using the WAVE extension on Chrome.
Changes made were to the
<main>landmarks and heading tags, with the aim of keeping the pages visually unchanged. The only significant difference is that the results and import pages now display the imported file name in a<h1>tag:Note that the
<h1>tags for the Anomaly Explorer and Single Metric Viewer pages are inside<EuiScreenReaderOnly>tags. The heading and nav menu structure of these pages are currently being reviewed, which may result in these page headings being made visible.Checklist
For maintainers