[Docs only] Various fixes/cleanups with demo code imports #5641

cee-chen · 2022-02-15T22:27:18Z

Summary

List of fixes/improvements in this PR:

Fixed CodeSandbox links not using the correct demo code with cleaned up/ordered imports
Fixed the import regex not recognizing @emotion/react as a valid import
Fixed line comments before imports (original PR, see below sub-section)
Accounted for the edge case where code might not have an EUI import in it
Cleaned up/enforced more consistent newlines between imports
Misc cleanup & tech debt (comment headings, removed unnecessary escapes, and added unit tests)

Import comment fix

Certain 3rd-party imports had location-sensitive comments directly above them, e.g. // @ts-ignore or // eslint-disable. Those comments were not getting caught by our new import regex added in #5533 and were falling out of order.

Example: https://elastic.github.io/eui/#/tabular-content/data-grid-ref-methods (click on Demo TS tab)

Before

After

Checklist

Added documentation
Checked Code Sandbox works for any docs examples

src-docs/src/components/guide_section/_utils.js

kibanamachine · 2022-02-15T23:05:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5641/

cchaos · 2022-02-15T23:08:09Z

Also, was the order of the React imports vs EUI imports ever discussed in #5533? Maybe it's just what I'm familiar with, but it feels odd to have EUI first, then React especially since EUI depends on React. Didn't know if there might be a quick fix for this.

cee-chen · 2022-02-16T17:21:53Z

@chandlerprall Any thoughts on Caroline's comments above? I have no strong feelings either way personally, I can kind of follow the reasoning that since EUI is the primary library that the demo code is intending to show, it's fine that the EUI imports come first. But like I said, I don't super mind either way and it would be a relatively quick change.

breehall · 2022-02-16T17:24:17Z

Also, was the order of the React imports vs EUI imports ever discussed in #5533?

This wasn't something that was originally discussed and likely happened because I didn't change the order of the original logic in this file, I just cleaned it up. I don't think I have any strong feelings either way and would be fine with the order change.

chandlerprall · 2022-02-16T17:31:24Z

I've no strong feelings either, but I do prefer importing react first. Would be a nice change if it's quick & straightforward 👍

kibanamachine · 2022-02-16T17:55:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5641/

…roperly grouped with other imports

- remove unnecessary escapes - remove unnecessary catches for multiple spaces - prettier will fix those

- it's unlikely, but otherwise the code outputs `import { } from '@elastic/eui';` which we don't want

…orts that come before

kibanamachine · 2022-02-17T00:05:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5641/

…(??) - c4329bc hadn't fixed every edge case/import permutation, so I wrote some unit tests to make sure I'd fully captured everything

cee-chen · 2022-02-17T02:33:05Z

welp, this was a pretty textbook case of going down a rabbit hole 🐰

I went a little extra and wrote unit tests for this file since it was getting pretty complex. @chandlerprall any chance you can help me out with running yarn jest on files inside ./src-docs? Right now the error it's giving is something about not being able to find ./scripts/babel/proptypes-from-ts-props from the babelrc config. I manually commented those lines locally to get my tests running, but it would be nice to run them automatically

the rabbit hole goes deeper...

kibanamachine · 2022-02-17T03:26:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5641/

breehall

This looks good to me! Thank you for adding tests for the utils file. It's become more complex with our recent changes, so the tests are a great addition!

chandlerprall

LGTM! Tested in the preview, changes look like they apply as desired in the Demo TS tab, and the codesandbox initial file now opens correctly too.

Holding off on an approval until the local test configuration is updated (talked about the potential fix over zoom), but wanted to get everything else out of the way.

…ocs/ + configure Jest to look at src-docs components

- needed to dedent both texts, not just one

cee-chen · 2022-02-23T21:08:02Z

src-docs/.babelrc.js

 );
 baseConfig.plugins.splice(
  index + 1,
  0,
-  './scripts/babel/react-docgen-typescript'
+  `${__dirname}/../scripts/babel/react-docgen-typescript`


@chandlerprall I have no idea why this works but I cribbed it from parcel-bundler/parcel#1698 (comment) and it seems to solve the broken Jest tests 🙈 I also don't really know why this one needs ../ and the previous one doesn't, hahaa

FWIW though I ran yarn start, yarn test-unit, and yarn build-pack and all of them ran with no errors with this change (and the new regex unit tests do show up as expected in yarn test-unit 🎉)

kibanamachine · 2022-02-23T21:46:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5641/

chandlerprall

Confirmed the src-docs test is detected & executed,

cee-chen · 2022-02-24T04:51:31Z

If this causes unexpected babel shenanigans down the line please do feel free to blame/ping (bling, if you will) me!

cee-chen added the skip-changelog label Feb 15, 2022

cee-chen requested review from chandlerprall and breehall February 15, 2022 22:27

Account for // @ts-ignore before imports on Typescript demos

a473452

cee-chen force-pushed the docs/fix-ts-ignore-imports branch from b08b3c8 to a473452 Compare February 15, 2022 22:27

cee-chen commented Feb 15, 2022

View reviewed changes

src-docs/src/components/guide_section/_utils.js Outdated Show resolved Hide resolved

Add regex comment breakdown

54debb3

cee-chen added 9 commits February 16, 2022 13:29

Merge branch 'main' into docs/fix-ts-ignore-imports

714e377

Setup for React import regex - comment blocks, var naming

fb804b3

Add react import regex & position

f9f317e

Fix newline spacing between import groups

c4329bc

Fix missing @ character causing @emotion/react import not to be p…

11d2011

…roperly grouped with other imports

Clean up remaining import regex

1d3e5b9

- remove unnecessary escapes - remove unnecessary catches for multiple spaces - prettier will fix those

Fix CodeSandbox not using cleaned up demo source code

9d718ef

Handle source code with no EUI imports

de3040d

- it's unlikely, but otherwise the code outputs `import { } from '@elastic/eui';` which we don't want

Fix React imports that sit below other imports lines gobbling the imp…

7566542

…orts that come before

Fix consistent newline spacing between import groups once and for all…

2f10904

…(??) - c4329bc hadn't fixed every edge case/import permutation, so I wrote some unit tests to make sure I'd fully captured everything

cee-chen changed the title ~~[Docs only] Account for line comments before imports on demo JS/TS~~ [Docs only] Various fixes/cleanups with demo code imports Feb 17, 2022

Fix Typescript CodeSandboxes not opening directly to the demo file

f2142bf

the rabbit hole goes deeper...

breehall approved these changes Feb 22, 2022

View reviewed changes

chandlerprall reviewed Feb 22, 2022

View reviewed changes

cee-chen added 3 commits February 23, 2022 12:42

Merge branch 'main' into docs/fix-ts-ignore-imports

c4018d6

Fix babel config to not throw errors when running jest tests on src-d…

78c6c71

…ocs/ + configure Jest to look at src-docs components

remove inaccurate dedent comments

b9abdda

- needed to dedent both texts, not just one

cee-chen commented Feb 23, 2022

View reviewed changes

Missed a __dirName (??)

8de5313

chandlerprall approved these changes Feb 23, 2022

View reviewed changes

cee-chen merged commit bd60073 into elastic:main Feb 24, 2022

cee-chen deleted the docs/fix-ts-ignore-imports branch February 24, 2022 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Docs only] Various fixes/cleanups with demo code imports #5641

[Docs only] Various fixes/cleanups with demo code imports #5641

cee-chen commented Feb 15, 2022 •

edited

Loading

kibanamachine commented Feb 15, 2022

cchaos commented Feb 15, 2022

cee-chen commented Feb 16, 2022

breehall commented Feb 16, 2022 •

edited

Loading

chandlerprall commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

kibanamachine commented Feb 17, 2022

cee-chen commented Feb 17, 2022

kibanamachine commented Feb 17, 2022

breehall left a comment

chandlerprall left a comment •

edited

Loading

cee-chen Feb 23, 2022

kibanamachine commented Feb 23, 2022

chandlerprall left a comment

cee-chen commented Feb 24, 2022

[Docs only] Various fixes/cleanups with demo code imports #5641

[Docs only] Various fixes/cleanups with demo code imports #5641

Conversation

cee-chen commented Feb 15, 2022 • edited Loading

Summary

Import comment fix

Before

After

Checklist

kibanamachine commented Feb 15, 2022

cchaos commented Feb 15, 2022

cee-chen commented Feb 16, 2022

breehall commented Feb 16, 2022 • edited Loading

chandlerprall commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

kibanamachine commented Feb 17, 2022

cee-chen commented Feb 17, 2022

kibanamachine commented Feb 17, 2022

breehall left a comment

Choose a reason for hiding this comment

chandlerprall left a comment • edited Loading

Choose a reason for hiding this comment

cee-chen Feb 23, 2022

Choose a reason for hiding this comment

kibanamachine commented Feb 23, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Feb 24, 2022

cee-chen commented Feb 15, 2022 •

edited

Loading

breehall commented Feb 16, 2022 •

edited

Loading

chandlerprall left a comment •

edited

Loading