Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

chore(Editor): make editor fully functional #473

Merged
merged 16 commits into from
Dec 3, 2018
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Nov 14, 2018

This PR uses react-source-render and Babel to render examples from source.
This allows to resolve our existing problems with the live editing process.


Before

Blank page on fail.

before

After

after

@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@fa00dc0). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #473   +/-   ##
=========================================
  Coverage          ?   87.53%           
=========================================
  Files             ?       46           
  Lines             ?     1532           
  Branches          ?      219           
=========================================
  Hits              ?     1341           
  Misses            ?      186           
  Partials          ?        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa00dc0...4da7e72. Read the comment docs.

@DustyTheBot
Copy link
Collaborator

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

Generated by 🚫 dangerJS

@layershifter layershifter changed the title [WIP] chore(Editor): fix editor chore(Editor): fix editor Nov 28, 2018
@layershifter layershifter changed the title chore(Editor): fix editor chore(Editor): make editor fully functional Nov 28, 2018
import * as Stardust from '@stardust-ui/react'

export const babelConfig = {
plugins: ['proposal-class-properties', ['transform-typescript', { isTSX: true }]],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange, but typescript's preset is registered, but not working 🤔
Will fill an issue to Babel's repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, are we evaluating now the examples as js code?

Copy link
Contributor

@kuzhelov kuzhelov Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, at least till the moment when this issue will be fixed :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, all TS transforms are applied. I mentioned that use there the plugin:

plugins: [['transform-typescript', { isTSX: true }]],

Instead of the preset:

presets: ['typescript'],

However, everything is works.

@@ -125,23 +117,22 @@ class ComponentExample extends React.Component<ComponentExampleProps, ComponentE
}
const { themeName } = nextProps
if (this.state.themeName !== themeName) {
this.setState({ themeName }, this.renderSourceCode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

...knobs,
},
}),
this.renderSourceCode,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}),
}}
>
{this.renderExampleFromCode()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 much cleaner!

<Segment size="small" color="red" basic inverted padded secondary>
<pre>{error}</pre>
</Segment>
<SourceRender.Consumer>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether context consumer model is the one that fits this case, because as an alternative we might think about storing rendered result in a state and render by consuming stored value. Not sure if this context-driven indirection is necessary here

Copy link
Member Author

@layershifter layershifter Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context allows to avoid the storage of render related stuff in the state of ComponentExample and avoid cycling updates like this: render ComponentExample => render source =>update state => render ComponentExample (again).

Copy link
Contributor

@kuzhelov kuzhelov Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, cannot completely understand how it is different with the context.

Lets, for the sake fo simplicity, consider the following case: we have code pane and render pane, where code is mapped to rendered DOM.

Also, suppose that there is ComponentExample component who manages these two.

Initially code and rendered DOM are rendered at the corresponding panes. After that we start editing code:

State-based approach

The following will happen if ComponentExample's state will be used:

`RENDER PANE`
-----------------
`CODE PANE` -> content changed ->`code` evaluated to `state.renderedTree`, RERENDER

Context-based approach

So, one rerendering for state (two if this process will be debounced) - no extra unexpected cycles. How it is different from context-based way? What we will have is the following (under assumption that debouncing is removed, as we have it now):

`RENDER PANE`
-----------------
`CODE PANE` -> content changed -> evaluated to `state.renderedTree`, RERENDER with updated context provider

So, still one update, but why do we need context-based indirection?


Please, help me settle these things in my head. Thank you!

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look absolutely good to me, just left couple of questions to refine

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 29, 2018
@@ -297,40 +289,13 @@ class ComponentExample extends React.Component<ComponentExampleProps, ComponentE
)
}

private renderSourceCode = _.debounce(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no debouncing anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't need now it fact. Performance is good enough.

@kuzhelov kuzhelov added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Nov 30, 2018
@layershifter layershifter merged commit 11e36d5 into master Dec 3, 2018
@layershifter layershifter deleted the chore/fix-editor branch December 3, 2018 09:25
return execute(transpiledCode, {
REACT: React,
REACT_DOM: ReactDOM,
STARDUST_UI_REACT: Stardust,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to find where are we handling this now...

'@stardust-ui/react': Stardust,
lodash: _,
react: React,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add react-dom here

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants