-
Notifications
You must be signed in to change notification settings - Fork 838
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
Add codesandbox links to our documentation #2728
Conversation
@@ -9,7 +9,7 @@ import { | |||
EuiSpacer, | |||
} from '../../../../src/components'; | |||
|
|||
import { VISUALIZATION_COLORS } from '../../../../src/services/color/visualization_colors'; |
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.
Should we deprecate this now that we have real palettes?
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 thought about that too as I was messing with the palettes and I don't think it makes sense to have two ways to export the viz palette.
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.
Cool. I'll add it to our list then. They are definitely duplicative, and the new one is a more reliable method of pulling those values.
.map(x => x.groups.import) | ||
.reduce((deps, dep) => { | ||
// Hack because the docs are locked to a specific version of React Router | ||
deps[dep] = dep === 'react-router' ? '^3.2.5' : 'latest'; |
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.
Couldn't think of a way to pull this from package.json automatically?
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.
const pkg = require('../../../../package.json');
at the top of the file
and then replace '^3.2.5'
with pkg.devDependencies['react-router']
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiSpacer, | ||
// @ts-ignore |
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.
Gotta follow up on why this was needed.
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 don't think this is necessary
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.
^^ Can delete now?
Hi @snide, I was thinking if we could have the Codesandbox icon next to the |
@miukimiu That was my original idea, but people liked the discreet link. I was thinking to add a new prop to EuiCodeBlock for Easy enough to change if others feel the same, but I'll let the PR get reviews / merged first on all the regex crazyness first before getting to fiddly with the design. |
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.
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiSpacer, | ||
// @ts-ignore |
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 don't think this is necessary
.map(x => x.groups.import) | ||
.reduce((deps, dep) => { | ||
// Hack because the docs are locked to a specific version of React Router | ||
deps[dep] = dep === 'react-router' ? '^3.2.5' : 'latest'; |
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.
const pkg = require('../../../../package.json');
at the top of the file
and then replace '^3.2.5'
with pkg.devDependencies['react-router']
@thompsongl Looks like it's something happening only in the last couple versions of EUI. Checking it out. You can replicate by moving the EUI req away from latest. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
This is mostly reviewable if folks want to poke around. I think the |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
I think we have an issue with all the pages using a DisplayToggles.
I noticed that this is not working as expected. The file is generated correctly but the For example, you can try the first example which uses a DisplayToggles on the EuiComboBox page: It throws the following error: Here's another example (Date picker states): |
I've pushed a fix for the display toggle import issue, but trying to render the toggles gives this error:
because display_toggles.js has |
I think eventually the "Playground" will allow us to get rid of the DisplayToggles. Perhaps just ignore all those examples for now? |
I had them working before these latest commits / changes from CS. I'll double check on them and see what's up. It touches a lot of our examples, so i wanted to make sure they worked. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
OK. Fix was to just not use react-router for that link, an use a simple href. It acts as it did before. Ultimately we can use playgrounds for this stuff, but for the moment I think it's really important to spin up a CS for any form element really quicky, which is why I think it's worth the very minor inconvenience of having to use an href there. |
@cchaos also added a notice in our PR template as we mentioned. I think this one looks good to merge. TY to @chandlerprall for the regex fix. |
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.
👍 Aside from the awesomeness of quickly getting to edit the example, I like that the home page link to Codesandbox is now super generic, that faker works, and TS docs can import from /components
now. 🥳
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiSpacer, | ||
// @ts-ignore |
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.
^^ Can delete now?
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
retest |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2728/ |
This is passing, and I think I've gotten to the feedback. I'm going to do a merge since this one touches so many files and I want to avoid conflicts. Since it's a docs level change I think this is pretty safe. |
Summary
Adds dynamic code sandbox docs to our documentation.
How it works
faker
in data grid) it will add it to the sandbox.Limitations
Here is a list of examples that don't work and the reason. We should clean these up in separate PRs.
🙈 = doesn't show a link to CS, no one will see it break
💔 = brittle regex causes an error in CS
faker
💔 EuiCodeEditor breaks because of the way we import Brace depsFor review
Checklist
Props have proper autodocsAdded or updated jest tests