-
Notifications
You must be signed in to change notification settings - Fork 843
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
[Docs] Consolidated GuidePage and GuideTabbedPage into just GuideTabbedPage
#5887
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5887/ |
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.
LGTM! 🎉
Tested in Chrome, Firefox, Safari, and Edge.
<GuideTabbedPage | ||
title="Sizing" | ||
isBeta={!showSass} | ||
notice={<ThemeNotice />} | ||
showThemeLanguageToggle | ||
> | ||
<GuideSection> | ||
<GuideSection color="transparent"> |
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.
The first <GuideSection />
in sizing page doesn't start with color="subdued"
like in the other pages: typography, colors and borders. This process to change the color seems very manual.
So should we have a prop in <GuideTabbedPage />
called alternateSectionsColors
(or a better naming) and if the prop is true all <GuideSection />
will get the color alternated starting with subdued then transparent.
Something like:
// in src/components/guide_tabbed_page/guide_tabbed_page.tsx
<EuiPageContent
css={css`
${alternateSectionsColors &&`
> .euiPanel:nth-child(even) {
background-color: transparent;
}
> .euiPanel:nth-child(odd) {
background-color: ${panelSubduedColor};
}
`}
`}
/>
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.
Yeah it's meant to be pretty manual because it's intentionally not trying to be zebra striping. It's supposed to only shade sections with ThemeExamples and not theme values (see in screenshot above). If we try to just do it automatically like you have it will likely shift which section will get shaded if there aren't value sections like on page: https://eui.elastic.co/pr_5887/#/theming/sizing. I think it's better to default to plain transparent/white background, making us more mindful of when we shade the backgrounds. Otherwise, I can see us fighting against the auto-child shading.
If no one has any objections to merging these docs pages, I"ll go ahead and merge the PR in an hour. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5887/ |
Now GuideTabbedPage can do everything GuidePage could on top of supplying
pages
. This also now removes the page-tabbed version ofPlayground
(all of them have been converted to the example tab). There was really no reason why they needed to be separate and trying to keep them in sync was too manual.Everything should behave just as it did, just with one less layout to maintain.
The GuideSection has also been updated to normalize padding when using
color
.color=undefined
: This is our default basic example setup with no changes except little extra was added between examplescolor=defined
: Meaning, any value is manually passed it will get added vertical spacing.But only when in the larger breakpoints. Otherwise, all sections are white (because they're so large) and smaller areas like ThemeExamples will get shaded.
Checklist
[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for any docs examples[ ] Added or updated jest and cypress tests[ ] Checked for breaking changes and labeled appropriately[ ] Checked for accessibility including keyboard-only and screenreader modes[ ] Updated the Figma library counterpart[ ] A changelog entry exists and is marked appropriately