[ML] Fix modal titles

[jgowdyelastic]

An EUI change has caused modal titles to no longer be implicitly wrapped in a header tag.
This change adds a h2 to all modal titles.

Old

New

[elasticmachine]

Pinging @elastic/ml-ui (:ml)

[darnautov]

@jgowdyelastic I reckon it's semantically incorrect putting the h1 header in a dialog. There is supposed to be only one h1 on a page and we already have it specified per page, e.g. "Trained Models", "Anomaly Explorer" etc. Hence I believe the dialog should be either h2 or h3 depending on what heading levels are present on a page and what section a particular dialog belongs to. To make it consistent we can specify h2 for all modal windows, WDYT?

[jgowdyelastic]

@jgowdyelastic I reckon it's semantically incorrect putting the h1 header in a dialog. There is supposed to be only one h1 on a page and we already have it specified per page, e.g. "Trained Models", "Anomaly Explorer" etc. Hence I believe the dialog should be either h2 or h3 depending on what heading levels are present on a page and what section a particular dialog belongs to. To make it consistent we can specify h2 for all modal windows, WDYT?

The EUI examples use h1 (https://elastic.github.io/eui/#/layout/modal) but I agree that we should really only have one per page.
Updated to h2 in ca32f39

[darnautov]

LGTM

[peteharverson]

These changes look good, but it looks like titles aren't being styled correct for any EuiConfirmModal opened from an EuiFlyout.

For example:

Compared to one which hasn't been opened from a flyout:

@elastic/kibana-design is this a known issue for EuiConfirmModal opened from a flyout, or do we need to change the code in some way to ensure the title is styled correctly?

[1Copenut]

@peteharverson Thank you for the ping on our Slack channel. If you pass a bare string or HTML tag as the children to EuiModalHeaderTitle, it will automatically wrap in an H1, which is the desired outcome.

The EUI team is working on a documentation update to spell out using H1 for modal titles today. After some discussion and reviews, we're recommending modals use H1 because it's a significant shift in user context. We want to visually and semantically convey this is a break in the previous flow, and we need your (user's) attention on this window for a moment.

[jgowdyelastic]

@peteharverson Thank you for the ping on our Slack channel. If you pass a bare string or HTML tag as the children to EuiModalHeaderTitle, it will automatically wrap in an H1, which is the desired outcome.

@1Copenut Are you sure this is the case now? this is how it used to work, but after a recent eui change it no longer seems to do this, hence the reason we've created this PR.

[1Copenut]

@jgowdyelastic I took a second look and found the root cause. EUI PR #6321 removed a containing div and replaced it with an EuiTitle component on line 27 of the EuiModalHeaderTitle.

EuiTitle only clones its children prop and returns children with any additional CSS. When we (EUI) removed the div, the FormattedMessages were being rendered as is, without wrapping in an H1. Your changeset here restores the proper styling.

If there's time for another round of changes, I'd recommend swapping H2 for H1 to give semantic parity to the default EUI modal. Or if this patch needs to land quickly, can we add an improvement ticket to the backlog?

[jgowdyelastic]

@1Copenut ok thanks.
I've switched it back to h1.
Do you have any thoughts about the strange EuiConfirmModal behaviour Pete reported in the comment above. It appears confirm modal title is wrong only when triggering the confirm modal from a flyout.

[1Copenut]

Do you have any thoughts about the strange EuiConfirmModal behaviour Pete reported in the #147855 (comment) above. It appears confirm modal title is wrong only when triggering the confirm modal from a flyout.

Without seeing the source React code, I'm speculating a bit, but...

The EuiConfirmModal that has the small text might be passing a React component to the title prop, where the other one with larger (correct) text is passing a string to title. EuiConfirmModal passes title through to EuiModalHeaderTitle silently.

[peteharverson]

Without seeing the source React code, I'm speculating a bit, but...

The EuiConfirmModal that has the small text might be passing a React component to the title prop, where the other one with larger (correct) text is passing a string to title. EuiConfirmModal passes title through to EuiModalHeaderTitle silently.

Yes, looks like the ones where we have the small text is because we are passing <FormattedMessage> to the title to handle the internationalization. We'd have to replace this by passing a string, for example

    title={i18n.translate(
      'xpack.ml.timeSeriesExplorer.deleteAnnotationModal.deleteAnnotationTitle',
      {
        defaultMessage: 'Delete this annotation?',
      }
    )}

Trying this out now leads me to a design question - this looks a bit odd where there is just a title and no child component:

compared to this, where I added a child <p>:

@elastic/kibana-design what do you think? Should we add a child component for all our EuiConfirmModal where we currently only have a title?

[mdefazio]

@elastic/kibana-design what do you think? Should we add a child component for all our EuiConfirmModal where we currently only have a title?

@peteharverson I don't think we need to force adding in text here unless it adds more value for the user. (See EUI Modal Guidelines). We also try to avoid saying 'Are you sure' (Writing guidelines) .

[jgowdyelastic]

@peteharverson updated EuiConfirmModal titles in 87e0129

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 87e0129

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
ml	3.4MB	3.4MB	+47.0B

History

• 💚 Build #98726 succeeded 8aa5eff
• 💚 Build #96353 succeeded 41082b9
• 💚 Build #96028 succeeded ca32f39
• 💚 Build #96002 succeeded 7ae4aa4
• 💔 Build #95989 failed a49ee08

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

cc @jgowdyelastic

[peteharverson]

Tested latest changes and LGTM