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

[ML] Fix modal titles #147855

Merged
merged 6 commits into from
Jan 10, 2023
Merged

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Dec 20, 2022

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
image

New
image

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov
Copy link
Contributor

@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
Copy link
Member Author

@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

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@peteharverson
Copy link
Contributor

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

For example:

image

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

image

@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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Contributor

1Copenut commented Dec 21, 2022

@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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Contributor

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:

image

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

image

@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
Copy link
Contributor

mdefazio commented Jan 3, 2023

@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
Copy link
Member Author

@peteharverson updated EuiConfirmModal titles in 87e0129

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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

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

cc @jgowdyelastic

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest changes and LGTM

@jgowdyelastic jgowdyelastic merged commit 5c8b6f2 into elastic:main Jan 10, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 10, 2023
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Jan 13, 2023
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**
<img width="824" alt="image"
src="https://user-images.githubusercontent.com/22172091/208673148-3da918c4-f1d7-48c5-8afd-022b912b49fc.png">

**New**
<img width="857" alt="image"
src="https://user-images.githubusercontent.com/22172091/208673087-1b8cba03-504b-4505-ac65-0fcfcce4b99b.png">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:fix v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants