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 2 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>
`;
7 changes: 6 additions & 1 deletion src/components/modal/modal_header_title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,14 @@ export const EuiModalHeaderTitle: EuiModalHeaderTitleProps = ({
...rest
}) => {
const classes = classnames('euiModalHeader__title', className);
const renderChildren = () =>
React.Children.map(children, (child) => {
if (typeof child === 'string') return <h1>{child}</h1>;
return child;
});
return (
<div className={classes} {...rest}>
{children}
{renderChildren()}
Copy link
Contributor

@cee-chen cee-chen Dec 20, 2021

Choose a reason for hiding this comment

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

I'm not 100% on the typeof children, but I think this should be more robust. I'd try to avoid maping children if possible in the edge case that someone passes an array/map of strings, in which case you'd end up with multiple <h1>s (another accessibility violation)

Suggested change
const renderChildren = () =>
React.Children.map(children, (child) => {
if (typeof child === 'string') return <h1>{child}</h1>;
return child;
});
return (
<div className={classes} {...rest}>
{children}
{renderChildren()}
const childrenWithHeading = useMemo(() =>
if (typeof children === 'string') return <h1>{children}</h1>;
return children;
}, [children]);
return (
<div className={classes} {...rest}>
{childrenWithHeading}

Copy link
Contributor Author

@1Copenut 1Copenut Dec 20, 2021

Choose a reason for hiding this comment

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

Good call @constancecchen. I refactored using your recommended approach, and found what I think is a better way to reason about children being a string than typeof. Source article here: https://overreacted.io/why-do-react-elements-have-typeof-property/

</div>
);
};