-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiPageTemplates] Some fixes for BWC #6015
[EuiPageTemplates] Some fixes for BWC #6015
Conversation
- Added `height` as optional prop to all `euiYScroll` mixins
Preview documentation changes for this PR: https://eui.elastic.co/pr_6015/ |
…page_next/fix_deprecated
- Adds `mainProps` to EuiPageTemplate for allowing customization of things like `id` (for skip link) - Fixed docs site 404 illustrations - Removed old `euiBreakpoint` for new one - Fixed Screen reader live docs’ usage of EuiPageTemplate
Ok, this PR is ready for review. Once merged into the Feature branch, I'll do one final pass through the entire checklist including the final CL items and Kibana local test. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6015/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code and tested it locally in Chrome, Safari, and Firefox.
There's a local warning that must be resolved:
Warning: React does not recognize the `innerProps` prop on a DOM element.
I also added some suggestions and all the rest looks good to me. 🎉
@@ -0,0 +1,30 @@ | |||
import React, { useState } from 'react'; | |||
|
|||
import { EuiPageTemplate_Deprecated as EuiPageTemplate } from '../../../../src'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My attempt to fix this. Replace the following code:
const combinedImports = elasticImports.join(', '); |
With:
const combinedImports = elasticImports.includes('as')
? elasticImports.join(' ')
: elasticImports.join(',\n ');
But better @thompsongl comes with a better solution. 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
865cf85 should do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 Yoda best!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6015/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6015/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @cchaos! LGTM! 🎉
EuiPageTemplate
mainProps
to support customid
and other basic HTML attributes to be passed to the inner /<main>
content wrapperEuiPageSection
EuiPageTemplate.Sidebar
EuiPageTemplate.BottomBar
paddingSize
from being passed through since this affects all 4 sides and un-aligns the contents to the page contents. If consumers want to adjust, they can pass their ownstyle
overrides.css
block and addedoverflow: hidden
in case the contents contains negative margins (like flex groups) that are larger than the padding area.EuiPageTemplate__Deprecated
paddingSize
.Other
height
as an option to theeuiYScroll()
mixins to easily change from100%
to something else.euiBreakpoint
Emotion mixinChecklist
[ ] Checked in Chrome, Safari, Edge, and Firefox[ ] Checked Code Sandbox works for any docs examples[ ] Checked for breaking changes and labeled appropriately[ ] Updated the Figma library counterpart[ ] A changelog entry exists and is marked appropriately