-
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
[Docs] Add ThemeLanguages and new Breakpoints page #5227
[Docs] Add ThemeLanguages and new Breakpoints page #5227
Conversation
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.
This PR is ready for review. Don't be scared by the line count 😺 , that's mostly from the addition of JSON files. I'm also completely open to any optimizations. Feel free to push directly.
But please do read through the PR summary first where I detail decisions made.
export const theme_languages: THEME_LANGUAGES[] = [ | ||
{ | ||
id: 'language--js', | ||
label: 'JS', | ||
title: 'Language selector: CSS-in-JS', | ||
}, | ||
{ | ||
id: 'language--sass', | ||
label: 'Sass', | ||
title: 'Language selector: Sass', | ||
}, | ||
]; |
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 labels for these button are up for debate. Mainly that I couldn't think of anything better than JS
for the Emotion stuff.
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.
No strong opinions in here personally, the only thing I can even remotely think of is changing the label to CSS-in-JS
instead of just JS
which feels a little wordier and more awkward, but does has a specific keyword connotation over just "JS". I'd be fine either way tbh
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.
CSS-in-JS instead of just JS
Same thought, but also unsure if it's better
// width: '100%', // Applies a specific width | ||
// enlarge: true, // Increase text size compared to rest of cells |
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.
TODO: TS complains about these props even though they exist...
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 also need to work on the responsive version of this table. For now it works, just isn't optimized.
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.
Resolved TypeScript errors in 58a6f14
function renderBreakpoint(size) { | ||
return ( | ||
<EuiFlexGroup |
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.
This was just moved from the deleted sass/breakpoints
file
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
I'm looking into making the JSON file build automated |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
role="region" | ||
aria-label="EUI Docs app bar" |
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.
Ooh thanks for fixing this! The axe-devtools complaint about our sticky top bar always bugged me :) @1Copenut, do you have any extra suggestions for this region/label, or does this look good to you?
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 like this role="region"
addition. It frames that control bar nicely. What do you think about spelling out the name like "EUI Documentation theme and apps" ? Users who benefit the most from aria labels may not think of this in terms of a bar, but more of what it allows them to do or learn about EUI.
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's so much really lovely stuff going on in this PR 😍 I love the tab switching UX for JS vs Sass, and the new documentation looks really great (love the copy-able snippet!). Code-wise everything looked fantastic as well, everything was straightforward and easy to read/understand, and I really appreciated the tendency to break up code into smaller separate utilities/helpers etc.
I didn't find any issues other than super small nits, so this LGTM, but def feel free to wait for other reviews/approvals to merge!
export const theme_languages: THEME_LANGUAGES[] = [ | ||
{ | ||
id: 'language--js', | ||
label: 'JS', | ||
title: 'Language selector: CSS-in-JS', | ||
}, | ||
{ | ||
id: 'language--sass', | ||
label: 'Sass', | ||
title: 'Language selector: Sass', | ||
}, | ||
]; |
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.
CSS-in-JS instead of just JS
Same thought, but also unsure if it's better
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 love how the new theming section is coming together. The breakpoints page looks good and the JS/Sass button is a great idea.
Tested in chrome, safari, Edge, and Firefox. I also looked at code. LGTM! 🎉
Preview documentation changes for this PR: https://eui.elastic.co/pr_5227/ |
* Added a ThemeSelector for optionally showing/changing content based on JS or Sass theming language * Added some theme helper components like alert banners, values table, and example row * [Manual] Copied the outputs json version of Sass for easier consumption in docs rather that sass-loader * Rearranging sidenav to promote Theme section and adding “Breakpoints” * Fixed some routing and notices
As part of the CSS-in-JS effort, we now have several ways consumers can consume our theme tokens; via Sass, JS (Theme context), or JSON (exports our Sass tokens as a JSON object). Our docs are currently confusing as to where/how to consume these tokens and might be one of the reasons why still see some raw pixel values downstream.
This PR is just the first of many that will cleanup and consolidate these theme token pages to hopefully make it clearer how to consume them and in their language of choice. It sets up a lot of reused components and the first converted page for "Breakpoints" as an example.
This PR is against master because it is docs only and has no dependencies on the feature branch.
1. Added a ThemeSelector for optionally showing/changing content based on JS or Sass theming language
I've created a button toggle that can be visually turned on via a new prop
showThemeLanguageToggle
onGuidePage
. This toggle is hooked in the docs' ThemeContext and therefore the value of which can be consumed via:It currently saves the value of the button in local storage so as users come and go, their selection will remain (similar to the theme selector).
I've also added a tour for new visitors 😄
Notes
There could be debate whether this should live in the header like the theme selector or only down in the page header as it is here. I've opted for only showing it on the pages that utilize it (at least for now) since there are a lot more that don't than do. Eventually, (maybe when we're more overall converted) we can move it to a more global location.
2. Added some theme helper components like alert banners, values table, and example row
Under
src-docs/theme
I added acomponents
folder with additional shared components that I'll use throughout. This includes having one place for the banner alerts that look like:The other components help to build out the repetitive portions of the
values tables:
and example panels:
3
[Manual]Copied the output json version of Sass for easier consumption in docs rather than sass-loaderThis is a portion that I could use some engineering help with.So far we've been using the
!!sass-vars-to-js-loader!
method to import Sass files to read the values of some tokens. This only works if the token is as stright string or reference to a string. It cannot run functions or mixins. So those docs tended to be manual, and lightly kept up to date.What does do all those calculations is our JSON converter. Which also doesn't rely on a specific loading order. So I've setup a folder where
I manually copy/pastedthe JSON theme files are copied from thedist
tosrc-docs/themes/json
at build time. Now I can read all our Sass tokens directly from that and we can either setup a script to always output this on Sass updates or docs build.So in the Sass portion of the docs we can just grab them like:
4. Rearranged sidenav to promote Theme section and adding “Breakpoints”
Now to the actual visible changes to the docs pages, I've moved the new "Theming" section to right after the Guidelines section. The context-based theming and Sass pages are mostly untouched, but I did add a page for detailing "Breakpoints".
This is the one of the few theme detailing pages to come and is the simplest.
5. Global values will slowly become the "Customizer"
As I move the full explanation of all the tokens to their separate theme files, I am also simplifying the "Global values" page to be just the token name, type and value updater.
I will also be deleting those sections from the existing Sass page.
Checklist
[ ] Checked in mobileI'll follow up with better responsive value tables[ ] Checked Code Sandbox works for any docs examples[ ] Added or updated jest tests[ ] Checked for breaking changes and labeled appropriately[ ] A changelog entry exists and is marked appropriately