[Final Feature PR] Convert to remaining page components to Emotion#5948
Conversation
# Conflicts: # src/components/page/page_body/__snapshots__/page_body.test.tsx.snap # src/components/page/page_body/page_body.tsx
…ge/convert_to_emotion
and move new EuiPageSidebar to /page and export
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5948/ |
|
This PR is ready for review, just doing the final checks |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5948/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5948/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5948/ |
elizabetdev
left a comment
There was a problem hiding this comment.
LGTM! 🎉
Looking forward to having the feature PR merged! 😍
I just found some minor issues, but nothing special.
| }); | ||
|
|
||
| const _minHeight = grow ? `max(${minHeight}, 100vh` : minHeight; | ||
| const _minHeight = grow ? `max(${minHeight}, 100vh)` : minHeight; |
There was a problem hiding this comment.
The minHeight prop is defined as string, should it be instead CSSProperties['minHeight']? I think this would make this prop more consistent with other components similar props.
There was a problem hiding this comment.
The problem is that the CSSProperties version actually evaluates to MinHeight<string | number> and I can't accept a number since it isn't applied directly to the style attribute but interpolated into a string first. (Well, I guess I could accept it, but I'd have to add a lot other logic to append px if it is a number.) It just seemed most straight forward to restrict it to string only.
| margin-left: auto; | ||
| margin-right: auto; | ||
| width: ${width}; | ||
| max-width: ${PAGE_MAX_WIDTH}; |
There was a problem hiding this comment.
Should this max-width use a logicalCSS()?
There was a problem hiding this comment.
Good catch with all those. I forgot to go back and check. I'll update.
| const _minHeight = grow ? `max(${minHeight}, 100vh)` : minHeight; | ||
|
|
||
| const classes = classNames('euiPageTemplate', className); | ||
| const pageStyle = { |
There was a problem hiding this comment.
Should this pageStyle be converted to Emotion? So that the styles don't show in the style prop?
There was a problem hiding this comment.
I left it as a style property because it's values are affected by other things on the page and I worried about the effect it would have on Emotion styling changing and not updating quickly enough. Also it could mean that it changes the class on every render which isn't very effecient.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5948/ |
|
I'm going to merge this into the feature branch on green. Unfortunately, it's not actually the last PR.. 😢 I have to re-work the cloning method of EuiPageTemplate to better suit Kibana's implementation. So keep an eye out for a new one. 😬 |
🙌 This is the final feature PR that just converts the remaining components to Emotion or deprecates some old ones without converting.
Converted:
EuiPageEuiPageBodyDeprecated and not converted:
EuiPageContent->EuiPageContent_DeprecatedEuiPageContentBody_DeprecatedEuiPageContentHeader_DeprecatedEuiPageContentHeaderSection_DeprecatedEuiPageSideBar->EuiPageSideBar_Deprecated(use newEuiPageSidebarinstead)Moved:
page_template/sidebartopage/page_sidebarto replace the deprecated version_EuiPageSidebar->EuiPageSidebar_EuiPageSidebarProps->EuiPageSidebarPropsChecklist
[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes[ ] Updated the Figma library counterpart