Skip to content

[EuiModal] Wrapping header in H1 if a string is returned #5494

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.

Sign up for GitHub

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

Merged
1Copenut merged 5 commits into from
Dec 21, 2021
Merged

[EuiModal] Wrapping header in H1 if a string is returned #5494

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9449add
[EuiModal] Wrapping header in H1 if a string is returned
1Copenut Dec 20, 2021
4909dbb
Adding a changelog entry for PR 5494
1Copenut Dec 20, 2021
6a4d6a3
Refactoring children string checker to be more fault tolerant.
1Copenut Dec 20, 2021
179a9fa
Refactoring to use top level API isValidElement
1Copenut Dec 21, 2021
f665b8f
Moving changelog entry to main.
1Copenut Dec 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- Fixed an accessibility issue where `EuiDatePicker` time options did not have unique IDs ([#5466](https://github.com/elastic/eui/pull/5466))
- Fixed global and reset styles when using the legacy theme ([#5473](https://github.com/elastic/eui/pull/5473))
- Fixed `EuiSuperDatePicker` not passing `isDisabled` to `EuiAutoRefresh` ([#5472](https://github.com/elastic/eui/pull/5472))
- Fixed `EuiModalHeaderTitle` to conditionally wrap title strings in an H1 ([#5494](https://github.com/elastic/eui/pull/5494))
Copy link
Contributor

@thompsongl thompsongl Dec 21, 2021

Choose a reason for hiding this comment

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

Move up to main

Copy link
Contributor Author

@1Copenut 1Copenut Dec 21, 2021

Choose a reason for hiding this comment

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

Thanks @thompsongl. I'll make this change and merge when tests pass.

Copy link
Contributor

@cee-chen cee-chen Dec 21, 2021

Choose a reason for hiding this comment

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

Don't forget the "Enable auto-merge" option also! It'll auto merge for ya when tests pass


## [`43.1.0`](https://github.com/elastic/eui/tree/v43.1.0)

Expand Down
8 changes: 6 additions & 2 deletions src/components/modal/__snapshots__/confirm_modal.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ Array [
class="euiModalHeader__title"
data-test-subj="confirmModalTitleText"
>
A confirmation modal
<h1>
A confirmation modal
</h1>
</div>
</div>
<div
Expand Down Expand Up @@ -160,7 +162,9 @@ Array [
class="euiModalHeader__title"
data-test-subj="confirmModalTitleText"
>
A confirmation modal
<h1>
A confirmation modal
</h1>
</div>
</div>
<div
Expand Down
4 changes: 3 additions & 1 deletion src/components/modal/__snapshots__/modal_header_title.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ exports[`renders EuiModalHeaderTitle 1`] = `
class="euiModalHeader__title testClass1 testClass2"
data-test-subj="test subject string"
>
children
<h1>
children
</h1>
</div>
`;
9 changes: 7 additions & 2 deletions src/components/modal/modal_header_title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import React, { FunctionComponent, HTMLAttributes } from 'react';
import React, { FunctionComponent, HTMLAttributes, useMemo } from 'react';
import classnames from 'classnames';
import { CommonProps } from '../common';

Expand All @@ -20,9 +20,14 @@ export const EuiModalHeaderTitle: EuiModalHeaderTitleProps = ({
...rest
}) => {
const classes = classnames('euiModalHeader__title', className);
const childrenWithHeading = useMemo(() => {
if (children && !children.hasOwnProperty('$$typeof'))
return <h1>{children}</h1>;
return children;
}, [children]);
return (
<div className={classes} {...rest}>
{children}
{childrenWithHeading}
</div>
);
Copy link
Contributor

@cee-chen cee-chen Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(continuing convo from #5494 (comment))

Ahh actually I totally forgot about React.isValidElement (https://davidwalsh.name/react-isvalidelement)! I think it's a bit more readable and exactly what we're looking for (distinguishing JSX vs strings):

Suggested change
const childrenWithHeading = useMemo(() => {
if (children && !children.hasOwnProperty('$$typeof'))
return <h1>{children}</h1>;
return children;
}, [children]);
return (
<div className={classes} {...rest}>
{children}
{childrenWithHeading}
</div>
);
return (
<div className={classes} {...rest}>
{React.isValidElement(children) ? children : <h1>{children}</h1>}
</div>
);

LMK what you think!

Copy link
Contributor Author

@1Copenut 1Copenut Dec 21, 2021

Choose a reason for hiding this comment

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

This feels a lot cleaner, and is the method I was hoping for, but not finding. Works great, and is easy to reason about.

};