docs(Examples): use sources to render dedicated examples, separate contexts #644

layershifter · 2018-12-19T14:02:12Z

Refs #95, fixes #554.

This PR:

removes examples from exampleContext and will require only index.tsx files (Types/index.tsx, Variations/index.tsx)
creates a new context for *.source.json files and uses it in ExternalExampleLayout with SourceRender

…amples

layershifter · 2018-12-19T14:13:52Z

docs/src/components/ExternalExampleLayout.tsx

+          <>
+            {element}
+            {/* This block allows to see issues with examples as visual regressions. */}
+            {error && <div style={{ fontSize: '5rem', color: 'red' }}>{error.toString()}</div>}


It's a little clumsy, but it will show us broken examples as visual regressions.

DustyTheBot · 2018-12-19T16:37:22Z

	Warnings
⚠️	There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS

…b.com/stardust-ui/react into docs/use-source-render

kuzhelov · 2019-01-02T02:10:17Z

docs/src/utils/exampleContexts.ts

@@ -0,0 +1,13 @@
+/**
+ * Get the Webpack Context for doc site example groups.


nit: suggest to omit 'Get' verb as these exports just provide corresponding context modules, while 'get' action happens on the client side.

Also, while 'groups' is also correct, would suggest to rename this context to exampleIndexContext - it will be easier for the reader to immediately infer the files implied by this context, without any additional necessity to remind terminology that is used by docs.

Agree, renamed to exampleIndexContext and removed the get verb 👍

kuzhelov · 2019-01-02T02:19:26Z

docs/src/utils/exampleKebabNameToSourceFilename.ts

+  // button-example           => ButtonExample.source.json
+  // button-example-shorthand => ButtonExample.shorthand.source.json
+  return `${_.startCase(exampleKebabName)
+    .replace(/ /g, '')


we can be a bit more expressive here. The only source of the whitespace could be either leading or trailing ones of the initial kebab-cased name. So, we might eliminate them before transform will happen, like that:

const trimmedExampleKebabName = exampleKebabName.trim() return `${_.startCase(trimmedExampleKebabName) ... }...`

While this makes code more efficient, I would put efficiency as the least worthy benefit in this case - the primary goal of this move is rather to improve explicitness of where the spaces (that regex aims to eliminate) might happen from.

Proposed solution is not correct because _.startCase() will create spaces:

_.startCase('foo-bar') // => 'Foo Bar'

kuzhelov · 2019-01-02T02:24:02Z

docs/src/utils/index.tsx

-export { default as exampleContext } from './exampleContext'
-export { default as exampleKebabNameToFilename } from './exampleKebabNameToFilename'
+export { exampleGroupsContext, exampleSourcesContext } from './exampleContexts'
+export { default as exampleKebabNameToFilename } from './exampleKebabNameToSourceFilename'


if this is not hard to do (which it seems not), I would suggest to rename the reexported entry so it will match the name of the file it is imported from

kuzhelov · 2019-01-02T02:26:39Z

docs/src/components/ExternalExampleLayout.tsx

+  const exampleSource: ExampleSource = exampleSourcesContext(examplePath)
+
+  return (
+    <SourceRender


still not sure why we are relying on the React Context functionality to render source code - but this is definitely not the point of this PR, we could address this moment later (if necessary)

I'm looking for a better solution there.

kuzhelov · 2019-01-02T02:29:43Z

docs/src/components/ComponentDoc/ComponentExamples.tsx

      new RegExp(`\/${displayName}\/index\.tsx$`).test(path),
    )
    if (!indexPath) {
      return null
    }

-    const ExamplesElement = React.createElement(exampleContext(indexPath).default) as any
+    const ExamplesElement = React.createElement(exampleGroupsContext(indexPath).default) as any


code which also suggests that exampleIndexContext would aid readability here. With current name used it was necessary for me to verify that all the index files are bundled by exampleGroupsContext

kuzhelov · 2019-01-02T02:32:41Z

docs/src/components/ComponentDoc/ComponentExamples.tsx

@@ -82,23 +81,24 @@ export default class ComponentExamples extends React.Component<ComponentExamples
  )

  private testExamplesStructure(displayName: string, allPaths: string[]): string[] {


given the return value of this method, it seems to be a good idea to rename it to something that is more descriptive, like getMissingExamplePaths - as otherwise it is completely unclear what this method is supposed to return

kuzhelov · 2019-01-02T02:43:06Z

docs/src/components/ComponentDoc/ComponentExamples.tsx

@@ -82,23 +81,24 @@ export default class ComponentExamples extends React.Component<ComponentExamples
  )

  private testExamplesStructure(displayName: string, allPaths: string[]): string[] {
-    const examplesPattern = `\.\/\\w*\/${displayName}[\\w\/]*\/\\w+Example`
-    const allExamplesRegExp = new RegExp(`${examplesPattern}[\\w\.]*\.tsx$`)
+    const examplesPattern = `\.\/${displayName}[\\w\/]*\/\\w+Example`


it seems that there is a problem with the regular expression used here - specifically, with the following part of it:

[\w\/]

Here it seems that the intent is to match either word character or slash, but the problem is that idle slash at start makes this regex to match w character literally.

This one will work correctly:

[\w\\]

The same problem apparently happens to the rest of the expression, where double slash is used here: \\w+Example. Am I missing something?

I simplified it to:

const examplesPattern = `\./${displayName}/[\\w/]+Example`

examplesPattern is not regex expression, it's a regular string. I'm not sure that we need there regular expressions at all.

This is much simpler to analyze, thanks!

still there is a problem, unfortunately - and this comes from the fact that slashes are not escaped in the string expression. This is what string expression will be evaluated to: ./displayName/[w/]+Example (with displayName='displayName')

And this expression creates several problems:

each of the path delimiter slashes should be escaped

dot at the start will match any character (is it our intent, or we would like to match dot literally? could we narrow down the set of characters to match, if so?)

I would see the following string should be used instead:

// here I suppose that intent is to match dot character literally, // please, make any necessary corrections if it is not const examplesPattern = `[.][/]${displayName}[/][\w/]+Example`

Please, just verify once again this regular expression before merging these changes. Thank you!

Sorry, merged before this comment 😿

escaped in the string expression

They will be escaped in new Regexp():

const displayName = 'Label' const r = new RegExp(`\./${displayName}/[\\w/]+Example`) r.toString() // "/.\/Label\/[\w\/]+Example/" r.test('./Label/Types/LabelExample') // true

kuzhelov

have left several minor comments, but there is one important one that I would like to clarify: https://github.com/stardust-ui/react/pull/644/files#r244656294 . Other than that see these changes being good to merge 👍

…b.com/stardust-ui/react into docs/use-source-render

layershifter added the 🚧 WIP label Dec 19, 2018

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners December 19, 2018 14:02

docs(Examples): separate contexts, use sources to render dedicated ex…

bcb97e1

…amples

layershifter force-pushed the docs/use-source-render branch from 5f7a7ad to bcb97e1 Compare December 19, 2018 14:06

layershifter added 🚀 ready for review and removed 🚧 WIP labels Dec 19, 2018

layershifter commented Dec 19, 2018

View reviewed changes

layershifter changed the title ~~[WIP] docs(Examples): use sources to render dedicated examples, separate contexts~~ docs(Examples): use sources to render dedicated examples, separate contexts Dec 19, 2018

Merge branch 'master' into docs/use-source-render

c93098a

layershifter mentioned this pull request Dec 20, 2018

docs(Examples): refactor SourceManager, add toggle for languages #650

Merged

Merge branches 'docs/use-source-render' and 'master' of https://githu…

19aada0

…b.com/stardust-ui/react into docs/use-source-render

kuzhelov reviewed Jan 2, 2019

View reviewed changes

layershifter added 4 commits January 2, 2019 12:23

fixes to review comments

047834f

simplify regex

3526341

Merge branches 'docs/use-source-render' and 'master' of https://githu…

994b6e9

…b.com/stardust-ui/react into docs/use-source-render

restore changes

93e1bc0

kuzhelov approved these changes Jan 2, 2019

View reviewed changes

layershifter merged commit 32c4653 into master Jan 2, 2019

layershifter deleted the docs/use-source-render branch January 2, 2019 11:52

		@@ -0,0 +1,13 @@
		/**
		* Get the Webpack Context for doc site example groups.

		@@ -82,23 +81,24 @@ export default class ComponentExamples extends React.Component<ComponentExamples
		)

		private testExamplesStructure(displayName: string, allPaths: string[]): string[] {

docs(Examples): use sources to render dedicated examples, separate contexts #644

docs(Examples): use sources to render dedicated examples, separate contexts #644

Uh oh!

Conversation

layershifter commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustyTheBot commented Dec 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layershifter Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

layershifter commented Dec 19, 2018 •

edited

Loading

layershifter Jan 2, 2019 •

edited

Loading