Add code toggleability #150

tizmagik · 2016-06-06T22:01:16Z

Adds the ability to toggle the CodeMirror box:

This may help address #86 but I'm actually not sure of the performance gains because I do not have a large testcase like the one in that issue. Below is the timeline for the basic example in the repo. You can see that rendering has improved and I imagine on a larger example case it could make a dramatic benefit. Either way, you can see that the 'Scripting' time is the big time suck, but that's a whole different story for another PR.

Before

After

I've also just done a very simple if (showCode) { ... } check, so not sure if there's an even more effective way to gain a performance boost by doing it another way or not, but at least it's a start.

It defaults to showCode: false in this branch, but I can have it default to true so it's not a "breaking" change, or even add it as a config option. Let me know!

tizmagik · 2016-06-06T22:05:28Z

src/rsg-components/Playground/Playground.css

@@ -1,6 +1,5 @@
 .root {
 	composes: border from "../../styles/colors.css";
-	overflow: hidden;


Wasn't completely sure why this was here to begin with. I needed to remove it so that the show/hide code tab would look visually outside of the Preview box.

sapegin · 2016-06-07T07:23:48Z

src/rsg-components/Playground/Playground.css

+.codeToggle {
+	composes: font from "../../styles/common.css";
+	composes: border from "../../styles/colors.css";
+	composes: link from "../../styles/colors.css";


You can import multiple classes:

composes: border link from "../../styles/colors.css";

sapegin · 2016-06-07T07:27:51Z

Cooool, thanks!

I think the default value should be an option but I’m not sure what value it should have. I’d make a breaking change if it really improves performance.

/cc @mik01aj @MoOx

mik01aj · 2016-06-07T08:15:07Z

Looking at the graphs you sent, I don't notice any performance boost. :(

To me it looks like something is wrong, either with my analysis in #86 or with this PR. @tizmagik could you drill down to see what's the performance hotspot in your case? (I mean "after")

And from UX perspective: I would totally remove the "hide code" button if code is shown by default.

tizmagik · 2016-06-07T21:38:39Z

I would say we should hide by default. It makes for a much cleaner presentation. I can add it as a config option to show/hide by default (although that would unfortunately mean we'd have to pass down the config all the way to Playground.js). If the config is set to show code by default then I can remove the "hide code" button, if you feel strongly, although it may be a bit confusing to lose that ability.

As for performance, again this PR isn't so much to address performance as it is to clean up the presentation slightly. I would also suggest we consider taking this a step further and include the PropTypes as part of the code to be collapsed, but I would leave that for another PR.

As for performance hotspot, in the basic example, the performance hotspot is definitely the Scripting rather than the Rendering. Although Render was improved by ~28% -- I just don't know what that would mean with a larger component base. Would anyone with a large component base be able to check out my branch and try it out?

mik01aj · 2016-06-08T07:15:07Z

I would say we should hide by default. It makes for a much cleaner presentation.

Agree, but can we consider this a minor change? If not, the default has to stay as it is and we can change it in the next major version. I'm not sure if this kind of change is minor or major.

that would unfortunately mean we'd have to pass down the config all the way to `Playground.js``

No, we could pass just one option, no need for entire config here.

Render was improved by ~28%

Well, yes, but render is just 3% of total time, so this is not significant. I'll reproduce my measurement from #86, maybe something has changed in the meanwhile.

mik01aj · 2016-06-08T08:45:08Z

I repeated my experiment, see #86 (comment)

tizmagik · 2016-06-08T16:41:44Z

Agree, but can we consider this a minor change? If not, the default has to stay as it is and we can change it in the next major version. I'm not sure if this kind of change is minor or major.

I think it's fair to call this a minor change, as it doesn't remove any real functionality or change behavior adversely. It's simply a UI update really. But if you still feel uncomfortable about it, we can have the config change default to showCode = true, but I don't think that's ideal. For now, I'm going to default to hidden unless you guys feel strongly otherwise.

No, we could pass just one option, no need for entire config here.

I know. What I'm saying is that specific config option would need to be passed down all the way to Playground which I believe is 3 or 4 levels deep from where config is available. I'll see if I can figure out a clean way to do it, maybe context would be useful here.

Well, yes, but render is just 3% of total time, so this is not significant. I'll reproduce my measurement from #86, maybe something has changed in the meanwhile.

Yes, but I imagine for a large number of components the render performance gain would be commensurate. As for your run in #86 it does look like something has changed. I believe it may have been the performance improvements made in React 15.

sapegin · 2016-06-09T07:51:27Z

I’m fine with breaking change because we have one more coming — #134.

mik01aj · 2016-06-09T10:09:22Z

@tizmagik fair enough. I'm ok with hidden by default.

tizmagik · 2016-06-09T17:36:04Z

Would you guys be okay with implementing this via context types? I just don't like how many layers I needed to manually pass this config setting down, and I think in general it might clean things up to have config available via context for whatever components need it now and in the future.

sapegin · 2016-06-09T18:02:59Z

I like the idea of passing config via context. But I think renderer components should still receive everything via props to make replacing these components easier.

tizmagik · 2016-06-09T18:15:29Z

But I think renderer components should still receive everything via props to make replacing these components easier.

That is a very good point. I'll see if adding config to context and then pulling in the specific values needed at the renderer component level is much cleaner or not.

…howCode` from that `context.config`

tizmagik · 2016-06-10T22:17:48Z

Alright, I've implemented via context. Good thing that I did, because actually I discovered a few places where I forgot to pass it around. Now via context its much clearer where that config setting is actually used and how it's made available.

The other two config-based properties (title and highlightTheme) are still being passed around manually, but I can refactor those to grab from context in a future PR.

I think this PR is ready to be merged now.

sapegin · 2016-06-12T18:37:55Z

src/rsg-components/Playground/Playground.js

+	constructor(props, context) {
+		super(props, context);
+		const { code } = props;
+		const { config: { showCode } } = context;


const { showCode } = context.config would be much more readable.

sapegin · 2016-06-13T12:46:37Z

@tizmagik Could you please tweak that one line and I’ll merge the PR?

tizmagik · 2016-06-13T13:49:44Z

@sapegin agreed and tweaked! :)

Add code toggleability

b1c94be

tizmagik reviewed Jun 6, 2016
View reviewed changes

Use same border-radius value

c22e055

sapegin reviewed Jun 7, 2016
View reviewed changes

Multi-compose

37ea14a

tizmagik added 4 commits June 8, 2016 13:54

Add showCode config option, defaulting to false

0ee94a3

de-lint

8e5a834

Update docs with new option

98b6c4e

Fix node 0.12

f4d1022

Provide config on context starting from <Layout /> level. Grab `s…

4a3c3f4

…howCode` from that `context.config`

Missed one :)

0f4270b

sapegin reviewed Jun 12, 2016
View reviewed changes

destructuring tweak

91abfd6

sapegin merged commit 80ab7b8 into styleguidist:master Jun 13, 2016

tizmagik deleted the code-toggle branch June 13, 2016 14:30

tizmagik mentioned this pull request Jul 7, 2016

Optimizing page rendering via hiding or deactivating CodeMirror until interaction #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code toggleability #150

Add code toggleability #150

tizmagik commented Jun 6, 2016

tizmagik Jun 6, 2016

sapegin Jun 7, 2016

sapegin commented Jun 7, 2016

mik01aj commented Jun 7, 2016 •

edited

tizmagik commented Jun 7, 2016

mik01aj commented Jun 8, 2016 •

edited

mik01aj commented Jun 8, 2016

tizmagik commented Jun 8, 2016

sapegin commented Jun 9, 2016

mik01aj commented Jun 9, 2016

tizmagik commented Jun 9, 2016

sapegin commented Jun 9, 2016

tizmagik commented Jun 9, 2016

tizmagik commented Jun 10, 2016 •

edited

sapegin Jun 12, 2016

sapegin commented Jun 13, 2016

tizmagik commented Jun 13, 2016

Add code toggleability #150

Add code toggleability #150

Conversation

tizmagik commented Jun 6, 2016

tizmagik Jun 6, 2016

Choose a reason for hiding this comment

sapegin Jun 7, 2016

Choose a reason for hiding this comment

sapegin commented Jun 7, 2016

mik01aj commented Jun 7, 2016 • edited

tizmagik commented Jun 7, 2016

mik01aj commented Jun 8, 2016 • edited

mik01aj commented Jun 8, 2016

tizmagik commented Jun 8, 2016

sapegin commented Jun 9, 2016

mik01aj commented Jun 9, 2016

tizmagik commented Jun 9, 2016

sapegin commented Jun 9, 2016

tizmagik commented Jun 9, 2016

tizmagik commented Jun 10, 2016 • edited

sapegin Jun 12, 2016

Choose a reason for hiding this comment

sapegin commented Jun 13, 2016

tizmagik commented Jun 13, 2016

mik01aj commented Jun 7, 2016 •

edited

mik01aj commented Jun 8, 2016 •

edited

tizmagik commented Jun 10, 2016 •

edited