[EuiModal] Wrapping header in H1 if a string is returned#5494
[EuiModal] Wrapping header in H1 if a string is returned#54941Copenut merged 5 commits intoelastic:mainfrom 1Copenut:feature/tpierce-eui-5276
Conversation
| const renderChildren = () => | ||
| React.Children.map(children, (child) => { | ||
| if (typeof child === 'string') return <h1>{child}</h1>; | ||
| return child; | ||
| }); | ||
| return ( | ||
| <div className={classes} {...rest}> | ||
| {children} | ||
| {renderChildren()} |
There was a problem hiding this comment.
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)
| 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} |
There was a problem hiding this comment.
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/
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5494/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5494/ |
| const childrenWithHeading = useMemo(() => { | ||
| if (children && !children.hasOwnProperty('$$typeof')) | ||
| return <h1>{children}</h1>; | ||
| return children; | ||
| }, [children]); | ||
| return ( | ||
| <div className={classes} {...rest}> | ||
| {children} | ||
| {childrenWithHeading} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
(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):
| 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!
There was a problem hiding this comment.
This feels a lot cleaner, and is the method I was hoping for, but not finding. Works great, and is easy to reason about.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5494/ |
cee-chen
left a comment
There was a problem hiding this comment.
🎉 Love it!! Thanks for the awesome accessibility improvements!
thompsongl
left a comment
There was a problem hiding this comment.
LGTM after the changelog update
CHANGELOG.md
Outdated
| - 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)) |
There was a problem hiding this comment.
Thanks @thompsongl. I'll make this change and merge when tests pass.
There was a problem hiding this comment.
Don't forget the "Enable auto-merge" option also! It'll auto merge for ya when tests pass
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5494/ |
Summary
Add a check in the
modal_header_title.tsxfile to wrapprops.childrenin an H1 if a string is passed into the component. Anything else (not a string) is ignored by this check. Closes #5276.Checklist
Added documentation