update react perf docs (#8060) and (#6174) #8236

dhyey35 · 2016-11-08T16:08:44Z

Number 11 on checklist of issue #8060 -

Added example, console screenshot and link to live demo where user can open console and call Perf methods.

lacker · 2016-11-16T21:32:21Z

docs/docs/addons-perf.md

+
+//If Perf is not accessible from console i.e throws RefrenceError: 
+//Perf is not defined, create a global 
+window.Perf = Perf;


I don't think this is something we want to tell everyone to do, is it?

In this tutorial I am using only one file but in real projects developers use bable and webpack or other such tools. If we import Perf ES6 style in any file of our project we have to make it global to access it from console and @gaearon has mentioned it in issue #6174 . So I think we should let developers know solution to a common problem beforehand. What do you think ?

It has been mentioned in the solution of issue #6174 . So I think it should be included so developers know how to solve it when they run into reference error.

lacker · 2016-11-16T21:32:34Z

docs/docs/addons-perf.md

@@ -14,6 +14,10 @@ next: test-utils.html
 import Perf from 'react-addons-perf' // ES6
 var Perf = require('react-addons-perf') // ES5 with npm
 var Perf = React.addons.Perf; // ES5 with react-with-addons.js
+
+//If Perf is not accessible from console i.e throws RefrenceError: 


nitpicks: spaces after //, Refrence misspelled

lacker · 2016-11-16T21:33:25Z

docs/docs/addons-perf.md

@@ -32,7 +36,7 @@ See these articles by the [Benchling Engineering Team](http://benchling.engineer

 If you're benchmarking or seeing performance problems in your React apps, make sure you're testing with the [minified production build](/react/downloads.html). The development build includes extra warnings that are helpful when building your apps, but it is slower due to the extra bookkeeping it does.

-However, the perf tools described on this page only work when using the development build of React. Therefore, the profiler only serves to indicate the _relatively_ expensive parts of your app.
+However, the perf tools described on this page only work when using the development build of React. If you set NODE_ENV to _production_ than it will not work.  Therefore, the profiler only serves to indicate the _relatively_ expensive parts of your app.


NODE_ENV is only applicable when compiling yourself - you might be using a CDN version. So I think this sentence is better just left out, the right way to refer to this is "using the development build" rather than "setting NODE_ENV".

lacker · 2016-11-16T21:33:47Z

docs/docs/addons-perf.md

@@ -56,6 +60,33 @@ The following methods use the measurements returned by [`Perf.getLastMeasurement

 * * *

+## Example Usage:


no : needed

lacker · 2016-11-16T21:35:00Z

docs/docs/addons-perf.md

@@ -56,6 +60,33 @@ The following methods use the measurements returned by [`Perf.getLastMeasurement

 * * *

+## Example Usage:
+
+We will take the comments example from old official tutorial. The example code is availabe on [github](https://github.com/dhyey35/react-example-for-performance/blob/master/public/scripts/example.js).


Rather than referring to "the old official tutorial" how about just having a little bit of example code and then linking to somewhere that it's available as a html file where people can just drop into dev tools. It could be in the assets directory in the react website, I think that would make sense because we put some other html stuff there too.

Where should I put my code in order to get it into assets dir on react website as I dont have access to it ? Examples directory of repo ?

lacker · 2016-11-16T21:35:25Z

docs/docs/addons-perf.md

+//Using react-with-addons.js in ES5
+var Perf = React.addons.Perf;
+
+var Comment = React.createClass({


our example code we're using ES6 for now - could you make this the same style as the other docs?

lacker · 2016-11-16T21:35:48Z

docs/docs/addons-perf.md

+  rawMarkup: function() {
+    var md = new Remarkable();
+    var rawMarkup = md.render(this.props.children.toString());
+    return { __html: rawMarkup };


Could you avoid using this dangerous html stuff in example code?

lacker · 2016-11-16T21:36:22Z

docs/docs/addons-perf.md

+
+![](/react/img/docs/addons-perf.png)
+
+A **Live Example** of the above code is available on [react-example-for-performance](https://react-example-for-performance.herokuapp.com/)


It's best to avoid dependencies on new backends, especially a one-off herokuapp instance. Perhaps this could just be a static file on the react website itself.

Where should I put the code to get it into assets dir of react website ?

davidwparker

Comment about inconsistent code spacing.

davidwparker · 2016-12-10T22:34:34Z

docs/docs/addons-perf.md

+
+const Comment = (props) => {
+  return (
+     <div className="comment">


Minor: Looks like you have some inconsistent spacing here. (sometimes 2, sometimes 3)...

dhyey35 · 2016-12-26T15:09:53Z

@davidwparker Fixed the inconsistent spacing issue. @lacker can you please tell me where should i put my example code as to get it into assets directory of react website which you mentioned in above comment

It could be in the assets directory in the react website, I think that would make sense because we put some other html stuff there too.

gaearon · 2017-01-04T16:27:43Z

@lacker Can you please follow up on this?

lacker · 2017-01-11T20:20:07Z

Hey, sorry I had a bit of email bankruptcy over the holiday break. Assets are in their own directories under docs, so js assets are in https://github.com/facebook/react/tree/master/docs/js for example and they appear on the site under URLs like https://facebook.github.io/react/js/remarkable.min.js . There is a bunch of misc example-type stuff in there so I think a dependency would be better in there than on a separate site.

andys8 · 2017-01-21T18:02:39Z

docs/docs/addons-perf.md

+      <span>{props.children.toString()}</span>
+    </div>
+  )
+}


@dhyey35 How about dropping return?

const Comment = props => ( <div className="comment"> <h2 className="commentAuthor"> {props.author} </h2> <span>{props.children.toString()}</span> </div> );

andys8 · 2017-01-21T18:03:41Z

docs/docs/addons-perf.md

+  document.getElementById('content')
+);
+```
+Open the developer console in the browser and type `Perf.start()` than followed by the action you want to monitor eg: submitting a form. Than type `Perf.stop()` in the console and than type `Perf.getLastMeasurements()` to get the measurements .See the Reference below for more methods.


"than" vs. "then"

andys8 · 2017-01-21T18:04:33Z

docs/docs/addons-perf.md

+  document.getElementById('content')
+);
+```
+Open the developer console in the browser and type `Perf.start()` than followed by the action you want to monitor eg: submitting a form. Than type `Perf.stop()` in the console and than type `Perf.getLastMeasurements()` to get the measurements .See the Reference below for more methods.


the measurements .See the

Spacing in text

andys8 · 2017-01-21T18:08:40Z

docs/docs/addons-perf.md

@@ -14,6 +14,10 @@ next: test-utils.html
 import Perf from 'react-addons-perf' // ES6
 var Perf = require('react-addons-perf') // ES5 with npm
 var Perf = React.addons.Perf; // ES5 with react-with-addons.js
+
+// If Perf is not accessible from console i.e throws ReferenceError: 
+// Perf is not defined, create a global


Question for understanding: Should it be available globally by default? When will it be available and when not? What circumstances trigger the different states?

dhyey35 · 2017-01-29T12:59:25Z

@andys8 I have made the changes you suggested. Can you please let me know if they are okay or need any changes ?

@lacker I pushed the code to docs/css/perf-tutorial.css , docs/tutorial/perf-tutorial.html , docs/_js/perf-tutorial.min.js as docs/js is in the .gitignore file. Have I placed them correctly ?

andys8 · 2017-01-29T16:58:03Z

@dhyey35 Great, i appreciate your changes.

lacker · 2017-02-02T01:12:08Z

docs/docs/addons-perf.md

@@ -57,6 +62,34 @@ The following methods use the measurements returned by [`Perf.getLastMeasurement

 * * *

+## Example Usage
+
+We will take the [simple example]() of a form which can be used to submit comments and then listing all comments. The example code is availabe on [github](https://github.com/dhyey35/react-example-for-performance/blob/master/public/scripts/example.js).


Looks like one of the links is missing here?

lacker · 2017-02-02T01:20:31Z

Hmm, the location of these files seems kind of weird - you are putting them through the site's build system when really you don't want any of that, you just want a static file. docs/tutorial does not seem right because the other stuff there is markdown. How about just sticking it in the downloads directory as a single file, similar to single-file-example.html ?

dhyey35 · 2017-02-06T15:08:47Z

@lacker I removed the missing link which wasn't needed and made a single html file in downloads/perf-tutorial.html

Small stuff

lacker · 2017-02-07T22:00:20Z

Awesome, looks good! Thank you for writing this up!

dhyey35 · 2017-02-11T06:08:37Z

@lacker @gaearon @andys8 @davidwparker @zpao Thanks for helping me throughout the way and giving opportunity to beginners to contribute to such a big project. This was my first PR to a big project. You guys are awesome.

lacker · 2017-02-13T20:46:30Z

Thanks for making the world a slightly-better-documented place ;-)

gaearon

I appreciate the effort but I think this needs to be reworked to better match the rest of the docs. I'm going to revert this specific PR but I'll be happy to review a follow-up that addresses these specific concerns.

gaearon · 2017-02-13T23:35:42Z

docs/docs/addons-perf.md

+We will take simple example of a form which can be used to submit comments and then listing all comments. The example code is [available on GitHub](https://github.com/dhyey35/react-example-for-performance/blob/master/public/scripts/example.js).
+
+```javascript
+import { Component } from 'react'


This import is unused.

gaearon · 2017-02-13T23:36:35Z

docs/docs/addons-perf.md

+import { Component } from 'react'
+import Perf from 'react-addons-perf'
+
+const Comment = (props) => (


We don't use this style of declaring components anywhere in the docs. We used function everywhere because it's easier to understand for people new to ES6.

gaearon · 2017-02-13T23:36:54Z

docs/docs/addons-perf.md

+	  <span>{props.children.toString()}</span>
+	</div>
+ )
+//code...


What does this comment represent? I don't think it makes the example more clear.

gaearon · 2017-02-13T23:37:20Z

docs/docs/addons-perf.md

+//code...
+
+ReactDOM.render(
+  <CommentBox />,


This component is named CommentBox but the component declared above is called Comment. What is the reason for the discrepancy?

gaearon · 2017-02-13T23:38:03Z

docs/docs/addons-perf.md

+  document.getElementById('content')
+);
+```
+Open the developer console in the browser and type `Perf.start()`. Then perform the action you want to monitor, like submitting a form. Finally, type `Perf.stop()` and `Perf.getLastMeasurements()` to get the measurements. See the Reference below for more methods.


The user reading this will likely miss the initial window.Perf assignment because it's in another code snippet, and will get that error about Perf being undefined.

Perf.getLastMeasurements() is an extremely low level method and gives information that has very little value to the user unless they know how to interpret it. This is definitely not what we should be suggesting to the user as an introductory example.

gaearon · 2017-02-13T23:39:28Z

docs/downloads/perf-tutorial.html

+            {
+              author: "Kevin Lacker",
+              id: 1,
+              text: "I Love React"


I think we could have better more appropriate text than praising ourselves. 😉

gaearon · 2017-02-13T23:44:03Z

Apologies for reverting—I want to get this in, but I just don't think it works as is. Happy to review this if you address the requested changes.

* update react perf docs issue 8060 and 6174 * Grammar Small stuff (cherry picked from commit 994a0c8)

dhyey35 · 2017-02-14T05:47:53Z

@gaearon No problems, will address the requested changes. Should I submit a new PR or restore my branch perf-docs and push to it ?

gaearon · 2017-02-14T12:16:39Z

A new PR would be fine. 👍
Please tag me in it and I’ll review.

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 8, 2016

gaearon assigned lacker Nov 8, 2016

lacker reviewed Nov 16, 2016

View reviewed changes

lacker added GH Review: needs-revision and removed GH Review: review-needed labels Nov 16, 2016

zpao added the Component: Documentation & Website label Nov 17, 2016

dhyey35 force-pushed the perf-docs branch from 8fc9125 to 5f9b788 Compare December 2, 2016 07:14

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Dec 2, 2016

davidwparker reviewed Dec 10, 2016

View reviewed changes

dhyey35 force-pushed the perf-docs branch from dd2a530 to 0a5dc48 Compare December 26, 2016 15:03

andys8 suggested changes Jan 21, 2017

View reviewed changes

dhyey35 force-pushed the perf-docs branch 3 times, most recently from fd1b36f to 479de22 Compare January 29, 2017 12:56

lacker reviewed Feb 2, 2017

View reviewed changes

lacker added GH Review: needs-revision and removed GH Review: review-needed labels Feb 2, 2017

update react perf docs issue 8060 and 6174

a791f5e

dhyey35 force-pushed the perf-docs branch from 479de22 to a791f5e Compare February 6, 2017 14:48

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Feb 6, 2017

Grammar

fa24104

Small stuff

lacker merged commit 994a0c8 into facebook:master Feb 7, 2017

lacker added the Documentation: needs merge to stable label Feb 7, 2017

dhyey35 deleted the perf-docs branch February 11, 2017 05:54

gaearon reviewed Feb 13, 2017

View reviewed changes

gaearon removed the Documentation: needs merge to stable label Feb 13, 2017

gaearon mentioned this pull request Feb 13, 2017

Revert "update react perf docs (#8060) and (#6174)" #8997

Merged

gaearon pushed a commit that referenced this pull request Feb 13, 2017

update react perf docs (#8060) and (#6174) (#8236)

b53a582

* update react perf docs issue 8060 and 6174 * Grammar Small stuff (cherry picked from commit 994a0c8)

		@@ -56,6 +60,33 @@ The following methods use the measurements returned by [`Perf.getLastMeasurement

		* * *

		## Example Usage:


		![](/react/img/docs/addons-perf.png)

		A Live Example of the above code is available on [react-example-for-performance](https://react-example-for-performance.herokuapp.com/)

update react perf docs (#8060) and (#6174) #8236

update react perf docs (#8060) and (#6174) #8236

Uh oh!

Conversation

dhyey35 commented Nov 8, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwparker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhyey35 commented Dec 26, 2016

Uh oh!

gaearon commented Jan 4, 2017

Uh oh!

lacker commented Jan 11, 2017

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!

dhyey35 commented Jan 29, 2017

Uh oh!

andys8 commented Jan 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lacker commented Feb 2, 2017

Uh oh!

dhyey35 commented Feb 6, 2017

Uh oh!

lacker commented Feb 7, 2017

Uh oh!

dhyey35 commented Feb 11, 2017

Uh oh!

lacker commented Feb 13, 2017

Uh oh!

gaearon left a comment

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!