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.

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

Fix EuiSideBar with css props not being detected by EuiPageTemplate #6324

Merged
merged 4 commits into from
Oct 26, 2022
+110 −14
Merged

Fix EuiSideBar with css props not being detected by EuiPageTemplate #6324

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9154dfc
Fix issue where using a `css` prop on EuiPageTemplate.Sidebar causes …
cee-chen Oct 25, 2022
ec495cc
Add missing children unit tests
cee-chen Oct 25, 2022
2000309
changelog
cee-chen Oct 25, 2022
37d5ffb
[test] Confirm that non-component JSX does not cause obj errors
cee-chen Oct 26, 2022
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
70 changes: 70 additions & 0 deletions src/components/page_template/__snapshots__/page_template.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,76 @@ exports[`EuiPageTemplate bottomBorder is rendered as true 1`] = `
</div>
`;

exports[`EuiPageTemplate children detects sidebars and does not place them in the main EuiPageInner 1`] = `
<div
aria-label="aria-label"
class="euiPageTemplate testClass1 testClass2 emotion-euiPageOuter-row-grow"
data-test-subj="test subject string"
style="min-block-size:max(460px, 100vh)"
>
<div
class="emotion-euiPageSidebar-l"
style="min-inline-size:248px"
/>
<div
class="emotion-euiPageSidebar-l-component"
style="min-inline-size:248px"
/>
<main
class="emotion-euiPageInner-panelled-left"
id="EuiPageTemplateInner_generated-id"
/>
</div>
`;

exports[`EuiPageTemplate children renders all other types within the main EuiPageInner 1`] = `
<div
aria-label="aria-label"
class="euiPageTemplate testClass1 testClass2 emotion-euiPageOuter-row-grow"
data-test-subj="test subject string"
style="min-block-size:max(460px, 100vh)"
>
<main
class="emotion-euiPageInner"
id="EuiPageTemplateInner_generated-id"
>
<header
class="euiPageHeader emotion-euiPageHeader-l-border"
>
<div
class="emotion-euiPageHeaderContent-l"
>
<div
class="css-ljpjbj-flex-center"
>
A
</div>
</div>
</header>
<section
class="emotion-euiPageSection-grow-l-top-plain"
>
<div
class="emotion-euiPageSection__content-l-restrictWidth"
style="max-width:1200px"
>
B
</div>
</section>
<section
class="emotion-euiPageSection-grow-l-top-plain"
>
<div
class="emotion-euiPageSection__content-l-restrictWidth"
style="max-width:1200px"
>
C
</div>
</section>
</main>
</div>
`;

exports[`EuiPageTemplate is rendered 1`] = `
<div
aria-label="aria-label"
Expand Down
30 changes: 30 additions & 0 deletions src/components/page_template/page_template.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import React from 'react';
import { css } from '@emotion/react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';
import { shouldRenderCustomStyles } from '../../test/internal';
Expand Down Expand Up @@ -99,4 +100,33 @@ describe('EuiPageTemplate', () => {
});
});
});

describe('children', () => {
it('detects sidebars and does not place them in the main EuiPageInner', () => {
const component = render(
<EuiPageTemplate {...requiredProps}>
<EuiPageTemplate.Sidebar />
<EuiPageTemplate.Sidebar
css={css`
color: red;
`}
/>
</EuiPageTemplate>
);
expect(component).toMatchSnapshot();
expect(component.find('main').children()).toHaveLength(0);
});

it('renders all other types within the main EuiPageInner', () => {
const component = render(
<EuiPageTemplate {...requiredProps}>
<EuiPageTemplate.Header>A</EuiPageTemplate.Header>
<EuiPageTemplate.Section>B</EuiPageTemplate.Section>
<EuiPageTemplate.Section>C</EuiPageTemplate.Section>
</EuiPageTemplate>
);
expect(component).toMatchSnapshot();
expect(component.find('main').children()).toHaveLength(3);
});
});
});
28 changes: 14 additions & 14 deletions src/components/page_template/page_template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,20 +159,20 @@ export const _EuiPageTemplate: FunctionComponent<EuiPageTemplateProps> = ({
React.Children.toArray(children).forEach((child, index) => {
if (!React.isValidElement(child)) return; // Skip non-components

switch (child.type) {
case EuiPageSidebar:
sidebar.push(
React.cloneElement(child, {
key: `sidebar${index}`,
...getSideBarProps(),
// Allow their props overridden by appending the child props spread at the end
...child.props,
})
);
break;

default:
sections.push(child);
if (
child.type === EuiPageSidebar ||
child.props.__EMOTION_TYPE_PLEASE_DO_NOT_USE__ === EuiPageSidebar
) {
Comment on lines +162 to +165
Copy link
Contributor Author

@cee-chen cee-chen Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just to clarify a few things:

  1. Is this the most robust solution? Absolutely not.
  2. Is this a pretty okay but kinda janky solution to unblock Kibana that will cover 95% of use cases? Yep!

This PR really only fixes Emotion usage and not any other potential wrappers/HOCs/etc around EuiPageSidebar. However, I'm personally okay with this, at least for now. If we get more reports down the road or more wrapper use-cases, we can re-evaluate a more robust solution - e.g. recursing into grandchildren to check for type - but I'm loathe to add that level of over-engineering at this stage.

I also feel confident that the unit test I've included (which checks for presence of items within main) will catch any regressions, should Emotion's internals change.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least this one only asks nicely that you don't use it, unlike React's variable that would fire you

sidebar.push(
React.cloneElement(child, {
key: `sidebar${index}`,
...getSideBarProps(),
// Allow their props overridden by appending the child props spread at the end
...child.props,
})
);
} else {
sections.push(child);
}
});

Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/6324.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiPageTemplate` not recognizing child `EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props