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.

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

[Page Layouts] Adding content props to EuiPageHeader for pre-determined page layout patterns #4451

Merged
merged 35 commits into from
Feb 10, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 26, 2021

To make it easier for Elastic products to follow the new page layout patterns, this PR adds some content-specific props for passing in things like pageTitle, and tabs. The left side content is the most prescriptive because there really are only a few options for how the titling, description and/or tabs should be handled. However, there's a wide variety of ways our products will utilize the right side of the page header, so I've opted to keep that one less specific.

The new docs section explains it best.

Screen Shot 2021-01-26 at 18 10 09 PM

This PR doesn't quite handle any Amsterdam specific theming or how the page header interacts with other page components yet. That will be forthcoming.

Also, to note, in order to make the component easier to manage, there is a new component called EuiPageHeaderContent which just handles the display of the content-specific props.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

src-docs/src/views/page/playground.js Outdated Show resolved Hide resolved
src-docs/src/views/page/playground.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@thompsongl thompsongl self-requested a review January 28, 2021 18:15
@thompsongl
Copy link
Contributor

Both comments (somewhat) addressed in cchaos#39
Not at all happy with the current method to toggle simulated content, but wanted to be sure this is what you were thinking.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@thompsongl
Copy link
Contributor

👉 cchaos#40

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@cchaos cchaos marked this pull request as ready for review February 3, 2021 16:23
@snide
Copy link
Contributor

snide commented Feb 4, 2021

@cchaos PR for you cchaos#41

This is great!

I found the docs needed some clarity around the new components. I primarily wanted the main example to show off the full, common layout that you're driving for. I know this is just a temporary state till you add more to it, but the previous example only told a partial story and with big "family" docs like these, I think it's good to start big and then break down the components as you read. Before it seemed to jump around a bit in the order (first page and header, then page, then header, then back to page).

This gave me a moment to get familiar with the new code, which worked really well. Only commentary there is that maybe you might want to label rightSideContent as something more generic. Will it always be on the right? Never know! From a prop naming standpoint some of the props use prefixes: pageTitle vs. description and tabs. I don't think matters which, but best to stick with one or the other. Preference would be to just use title?

image

@cchaos
Copy link
Contributor Author

cchaos commented Feb 4, 2021

Thanks @snide ! I've gone through your PR and did a couple updates before merging as well.

Only commentary there is that maybe you might want to label rightSideContent as something more generic. Will it always be on the right? Never know!

Yes it will. 😉 There's no way for users not to have it be on the right.

pageTitle vs. description and tabs. I don't think matters which, but best to stick with one or the other. Preference would be to just use title

I had this thought too but that would conflict with the regular HTML DOM property title.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

I love the way you thought about all the props. It makes the EuiPageHeader much more flexible. 🎉

Tested in Chrome, Firefox, Edge, and Safari, and it looks good. I just have a few suggestions.

src-docs/src/views/page/page_example.js Outdated Show resolved Hide resolved
src-docs/src/views/page/page_example.js Outdated Show resolved Hide resolved
src/components/page/page_header/page_header_content.tsx Outdated Show resolved Hide resolved
src-docs/src/views/page/page_example.js Outdated Show resolved Hide resolved
src-docs/src/views/page/page_example.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@cchaos cchaos requested a review from myasonik February 8, 2021 19:02
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Just a couple small TS things and a question

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM after the CL update 👍

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4451/

@cchaos cchaos merged commit 0427fa5 into elastic:master Feb 10, 2021
@cchaos cchaos deleted the props/page_header branch February 10, 2021 20:40
@cchaos cchaos mentioned this pull request Aug 28, 2021
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants