[SharedUX] Migrate PageTemplate component #129323
Conversation
majagrubic
added
v8.3.0
Team:SharedUX
There was a problem hiding this comment.
To be honest, all these configurations still feel more complicated than they need to be. But since this is just a complete copy over from the kibana_react version, we can optimize later once I've finished all my work on EuiPageTemplate (still working through smaller PR's; gonna be a while). But I don't want to block getting the NoDataPage into Analytics.
Pushed a small design fixes commit
| tags: ['shared-ux', 'component'] | ||
| date: 2022-04-04 | ||
| --- | ||
| This component is a thin wrapper around `EuiTemplate`, providing Kibana-specific additions. It can be configured to display solution navigation bar, empty page and no data config page. |
There was a problem hiding this comment.
Since we're mostly copying right now, can we copy/paste the existing doc for KibanaPageTemplate into this file and update any references/screenshots?
There was a problem hiding this comment.
I have updated the .mdx file and made corresponding changes where needed (omitting "recommended" and changing the actions to action). We might want to update that last screenshot though, as it still shows 2 cards.
There was a problem hiding this comment.
🙏 Thank you! Here's an updated image:
There was a problem hiding this comment.
Images are loaded from dev_docs, so I opened a separate PR for that (the URL remains the same).
packages/kbn-shared-ux-components/src/page_template/page_template.test.tsx
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
packages/kbn-shared-ux-components/src/page_template/page_template.stories.tsx
Outdated
Show resolved
Hide resolved
| import { shallow } from 'enzyme'; | ||
| import { KibanaPageTemplateInner } from './page_template_inner'; | ||
| import React from 'react'; | ||
| import { EuiEmptyPrompt, EuiPageTemplate } from '@elastic/eui'; |
There was a problem hiding this comment.
nit: we need to get into the habit of sorting imports, with newlines between classifications. My preference is as follows, but yours may be different:
// imports from npm packages
import React from 'react';
import { shallow } from 'enzyme';
// imports from elastic packages
import { EuiEmptyPrompt, EuiPageTemplate } from '@elastic/eui';
// imports from relative directories
import { FooBar} from '../../../components/foo/bar';
// imports from immediate files
import { KibanaPageTemplateInner } from './page_template_inner';
| */ | ||
| const emptyStateDefaultTemplate = 'centeredBody'; | ||
| let header = pageHeader; | ||
| if (isEmptyState) { |
There was a problem hiding this comment.
| if (isEmptyState) { | |
| if (isEmptyState) { |
| children = ( | ||
| <EuiEmptyPrompt | ||
| iconType={iconType} | ||
| iconColor={''} // This is likely a solution or app logo, so keep it multi-color |
There was a problem hiding this comment.
| iconColor={''} // This is likely a solution or app logo, so keep it multi-color | |
| iconColor="" // This is likely a solution or app logo, so keep it multi-color |
| <EuiEmptyPrompt | ||
| iconType={iconType} | ||
| iconColor={''} // This is likely a solution or app logo, so keep it multi-color | ||
| title={pageTitle ? <h1>{pageTitle}</h1> : undefined} |
There was a problem hiding this comment.
We shouldn't do this inline. Object and function creation in a prop leads to recreation on every render cycle.
const title = pageTitle ? <h1>{pageTitle}</h1> : undefined;
const body = description ? <p>{description}</p> : undefined;
children = (
<EuiEmptyPrompt
iconColor={''}
actions={rightSideItems}
{...{ iconType, title, body }}
/>
);
| paddingSize: 'none', | ||
| ...props.pageSideBarProps, | ||
| className: sideBarClasses, | ||
| } as EuiPageSideBarProps; // needed because for some reason 'none' is not recognized as a valid value for paddingSize |
There was a problem hiding this comment.
I'll look into this.
| <WrappedComponent | ||
| {...propagatedProps} | ||
| pageSideBar={pageSideBar} | ||
| pageSideBarProps={pageSideBarProps} |
There was a problem hiding this comment.
nit: foo={foo} always strikes me as repetitive and noisy, especially when there are several and you already have a spread.
| </WrappedComponent> | ||
| ); | ||
| }; | ||
| WithSolutionNav.displayName = `WithSolutionNavBar(${getDisplayName(WrappedComponent)})`; |
There was a problem hiding this comment.
| WithSolutionNav.displayName = `WithSolutionNavBar(${getDisplayName(WrappedComponent)})`; | |
| WithSolutionNav.displayName = `WithSolutionNavBar(${getDisplayName(WrappedComponent)})`; | |
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
miscellaneous assets size
History
To update your PR or re-run it, just comment with: |
Summary
This is the last PR in the series of migrating PageTemplate components. It migrates the actual
PageTemplatecomponent fromkibana_react, adds more tests and storybook files. There are no major changes in the functionality.Checklist
Delete any items that are not applicable to this PR.
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)For maintainers