Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Docs] Docs Bugs & Issues #8035

Closed
7 of 15 tasks
gaearon opened this issue Oct 21, 2016 · 46 comments
Closed
7 of 15 tasks

[New Docs] Docs Bugs & Issues #8035

gaearon opened this issue Oct 21, 2016 · 46 comments
Assignees

Comments

@gaearon
Copy link
Collaborator

gaearon commented Oct 21, 2016

We just completely rewrote docs in #7864.
They're bound to cause some issues, have some mistakes, etc.
Let's keep this issue as an umbrella for all problems we're discovering after the initial rollout.

  • Old links shouldn't be 404! Major issue.
  • Topbar points to https://facebook.github.io/react/index.html which is ugly URL
  • We shouldn't use Number as an identifier docs: list and key examples override Number #8034
  • Remove vars everywhere except ES5 page
  • JSX in depth has weird names like Story1, render2 @lacker
  • JSX in depth shows a failing example with <hello> tag but doesn't show how to fix it @lacker
  • Localized pages are gone: need to set up a new streamlined localization effort (cc @thejameskyle)
  • Mention in default index.html that it is not production-ready
  • Mark mockComponent as legacy and unnecessary
    Don't make mockComponent depend on Jest #2499 (comment)
  • shallowCompare on Addons page should recommend PureComponent instead
  • Figure out what to do with Community section ([Docs] Community section #6099)
  • Mention string refs
  • Forms doc has mistakes
  • Forms doc uses setState for uncontrolled components which is confusing
  • Installation is messy
@luqmaan
Copy link

luqmaan commented Oct 21, 2016

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 21, 2016

Yea, something happened to all some redirects. @lacker

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 21, 2016

@luqmaan Thanks, fixed. Let us know if you find more broken links.

@hzoo
Copy link
Contributor

hzoo commented Oct 21, 2016

On Hello World, https://facebook.github.io/docs/installation.html should be https://facebook.github.io/react/docs/installation.html so the react/ isn't added and redirects to https://code.facebook.com/?

[Installation](/docs/installation.html) page

@johnbrett
Copy link

Great job on new docs all, love the live editor with error handling.

Just curious: Any reason why var is used on all the homepage tutorials? Curious whether there's a "best practice" on this.. would have thought const where possible, otherwise let?

@hzoo
Copy link
Contributor

hzoo commented Oct 21, 2016

Another thing I noticed: this might of been an issue with the old docs but syntax errors don't have correct formatting of newlines from babel-code-frame

screen shot 2016-10-21 at 5 45 35 pm

Someone can make a pr to add to add style={{whiteSpace: "pre-wrap"}} as a style on the div at

<div className="playgroundError">{err.toString()}</div>,
or edit the .playgroundError css class in https:/facebook/react/blob/15-dev/docs/css/react.scss (<pre> seems to make it go over the width limit of the colored error box)

screen shot 2016-10-21 at 5 47 37 pm

Width would need to be longer to be accurate but it would be better than before

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 21, 2016

Any reason why var is used on all the homepage tutorials?

No reason, feel free to send a PR fixing this.

@pythononwheels
Copy link

If this is the "new style" I think you should point out the key differences to the "old style".
Just to take people with you.... knowing what key things change and why ...

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 21, 2016

Does it matter at this point? It's not as much style as a lack of cohesion in previous docs.

They weren't really grouped by topic, there was no clear progression from simple to complex, their titles weren't descriptive, there were many random "tips" that didn't fit a cohesive narrative, they were using APIs that aren't used widely in the ecosystem anymore, etc.

New docs are in line with how people use React today, have descriptive titles, and a linear progression from simple to more complex topics.

That's the basic difference.

@p-adams
Copy link

p-adams commented Oct 22, 2016

Possible typo in docs: "Consider the ticking clock example from the one of the previous sections."

https://facebook.github.io/react/docs/state-and-lifecycle.html

@hnordt
Copy link
Contributor

hnordt commented Oct 22, 2016

@gaearon the uncontrolled form example on https://facebook.github.io/react/docs/forms.html#uncontrolled-components is passing value, so it's actually a controlled example.

The correct example would be something like:

class Form extends React.Component {
  constructor(props) {
    super(props);
    this.handleSubmit = this.handleSubmit.bind(this);
  }

  handleSubmit(event) {
    alert("Text field value is: '" + this.textInput.value + "'");
  }

  render() {
    return (
      <div>
        <input
          type="text"
          placeholder="Hello!"
          ref={(input) => this.textInput = input}
        />
        <button onClick={this.handleSubmit}>Submit</button>
      </div>
    );
  }
}

ReactDOM.render(<Form />, document.getElementById('root'));

@giuseppeg
Copy link
Contributor

giuseppeg commented Oct 22, 2016

A controlled component does not maintain its own internal state; the component renders purely based on props.

  • is a bit confusing, probably better clarify that we are talking about DOM state instead of React state.


  • In jsx-in-depth.html#javascript-expressions there are two headings with the same ID "JavaScript Expressions" - this makes it impossible to link to the second one.
  • The fourth example in the innermost one is missing props.:
// Calls the children callback numTimes to produce a repeated component
function Repeat(props) {
  var items = [];
  for (var i = 0; i < numTimes; i++) {
  // ... 
}

Here numTimes should be props.numTimes.

@Andreyco
Copy link

Andreyco commented Oct 22, 2016

Why was info about latest stable removed from landing page?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 22, 2016

@Andreyco What do you mean? There was a "Download Starter Kit" link that mentioned version but most people didn't find that "starter kit" useful. You can always find the latest version on Releases GH page.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 22, 2016

Another feedback on "Controlled Components": #8052

@Andreyco
Copy link

@gaearon Yes, I mean link that mentioned version. I don't care about link, I am interested in version information. Anyway, no big deal, I will search for latest in GH releases. TY.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 22, 2016

Merged a bunch of PRs with fixes. Keep them coming please!

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

https://facebook.github.io/react/blog/2016/07/22/create-apps-with-no-configuration.html looks very weird on mobile, with some sort of a broken top bar

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

Figure out if we do redirects correctly https://twitter.com/spiceee/status/789774523549745152

@gaearon gaearon changed the title [Docs] New Docs Bugs & Issues [New Docs] Docs Bugs & Issues Oct 23, 2016
@giuseppeg
Copy link
Contributor

@gaearon have you considered to replace redcarpet with kramdown?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

@giuseppeg No idea, want to send a PR so I better understand the difference?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

More feedback: people are used to seeing version number on homepage. Can we add it somewhere and also link to the changelog for good measure?

@simonsmith
Copy link

simonsmith commented Oct 24, 2016

Just reading though the "React Without ES6" page:

In React components declared as ES6 classes, methods follow the same semantics as regular ES6 classes. This means that they don't automatically bind this to the instance. You'll have to explicitly use .bind(this) in the constructor:

Perhaps it's worth mentioning that this only applies in event handlers. Otherwise this works just fine. Happy to open a PR if so.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 24, 2016

@simonsmith Sounds good.

@aweary
Copy link
Contributor

aweary commented Oct 24, 2016

Perhaps it's worth mentioning that this only applies in event handlers. Otherwise this works just fine. Happy to open a PR if so.

@simonsmith @gaearon I don't think that's true if I'm understanding you correctly. You need to bind your method anytime it might be executed in a context where this does not refer to the component instance it was defined on.

class Bar extends React.Component {
  constructor(props) {
    super(props);
    // this will throw since it tries to access this.state.foo and
   // this.state is undefined on Bar
    this.props.logFoo();
  }
   render() {
    return <h1>Bar</h1>
  }

}

class Foo extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
        foo: "foo"
    };
  }
  logFoo() {
    console.log("Foo is: ", this.state.foo);
  }
  render() {
    return <Bar logFoo={this.logFoo} />
  }
}

An event handler is the most common case, but it's not the only case where you need to bind methods.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 24, 2016

The rule of thumb is that if you access method anywhere without () right after it, then you need to bind it.

@aweary
Copy link
Contributor

aweary commented Oct 24, 2016

The rule of thumb is that if you access method anywhere without () right after it, then you need to bind it.

Is that in the docs? If not, it should be 😄

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 24, 2016

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 24, 2016

PRs welcome!

@simonsmith
Copy link

simonsmith commented Oct 24, 2016

@aweary Yeah, my point was the docs imply you need to use bind whenever you want to use an instance method which (as you've demonstrated) is only true some of the time.

I've seen code where every method was bound by hand even if it was just being called normally. I can see why people might think that was correct. I'll put together some changes and we can discuss in a PR 👍

@Shenlok
Copy link

Shenlok commented Oct 25, 2016

No mention of the old string-based refs in the new docs. Should these be mentioned in a "deprecated" notice at least?

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 25, 2016

@Shenlok They're technically not deprecated yet, just considered legacy.

Refs and the DOM does mention them but too briefly:

Using the ref callback just to set a property on the class is a common pattern for accessing DOM elements. If you are currently using this.refs.myRefName to access refs, we recommend using this pattern instead.

I agree we should add a small section at the bottom at least describing how string refs work, with a notice that we don't recommend using them. Could you send a PR for this?

@SMHFandA
Copy link
Contributor

Forms docs has mistakes.
In "Forms" docs i found mistake in "Basic Radio Button" example. Method render() do not use this.state.value, but state has it. According "State and Lifecycle" docs it is bad:

While this.props is set up by React itself and this.state has a special meaning, you are free to add additional fields to the class manually if you need to store something that is not used for the visual output.
If you don't use something in render(), it shouldn't be in the state.

@julen
Copy link
Contributor

julen commented Oct 31, 2016

(Sorry if this has been discussed elsewhere — I haven't been able to find any references to it though).

there were many random "tips" that didn't fit a cohesive narrative

I agree there was no narrative here, although at the same time many of these provided really good advice on anti-patterns and things to avoid. At least they have been very valuable to me on the way to learning React.

Now as I browse/search through the new docs, I am missing these and I am not sure whether they have been integrated under other sections or discarded by some reason. IMHO having a specific section on anti-patterns can be beneficial for newcomers to avoid making common mistakes (e.g. using props as initial state), as well as a reference to be able to point to others.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 8, 2016

@julen All valuable tips were integrated into the relevant content. There aren't many "antipatterns" in React despite what some people say. If something is "bad" we try to disallow it on the API level.

@gaearon gaearon self-assigned this Nov 9, 2016
@jackjocross
Copy link
Contributor

Tiny (possible?) typo: in reference-react.md, the code for React.Children.count omits React which could be slightly confusing to newcomers.

@joni-
Copy link

joni- commented Dec 25, 2016

I was wondering about the example code in https://facebook.github.io/react/docs/thinking-in-react.html#step-4-identify-where-your-state-should-live.

I don't really understand why the filtering should be ProductTable's responsibility. Wouldn't it make more sense to do the filtering in FilterableProductTable and just pass the filtered products down to ProductTable?

@kalmanh
Copy link

kalmanh commented Feb 9, 2017

@gaearon @aweary I know I am "beating a dead horse" on this (as a similar issue was discussed above in this thread), but after reading the following in the docs in handling events

You have to be careful about the meaning of this in JSX callbacks. In JavaScript, class methods are not bound by default. If you forget to bind this.handleClick and pass it to onClick, this will be undefined when the function is actually called.

This is not React-specific behavior; it is a part of how functions work in JavaScript.

you walk away thinking that you have to bind hi() and callAnother() methods in the following code

class Test {
    constructor(){
        this.someProperty = "this is a test";
    }

    hi(){
     console.log(`this in hi() =  ${JSON.stringify(this)}`);
     this.callAnother();
    }

    callAnother(){
     console.log(`this in callAnother() = ${JSON.stringify(this)}`);
    }
}

const c = new Test();
c.hi();

which is obviously not true.

So, what the paragraph is really trying to say is this, but it needs to do it better... :)

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 9, 2017

@kalmanh Send a PR ;-)

@kkemple
Copy link

kkemple commented Mar 20, 2017

Tiny (possible?) typo: in reference-react.md, the code for React.Children.count omits React which could be slightly confusing to newcomers.

@jayjaycross did you want to fix that typo? If not I'll create a PR

@jackjocross
Copy link
Contributor

@kkemple go for it!

@ivstas
Copy link

ivstas commented Oct 7, 2017

Not sure it is a proper place to post it, but "React documentation is Creative Commons licensed." in the end of README.md leads to 404 github page.

@monkindey
Copy link
Contributor

monkindey commented Oct 8, 2017

yeah, I think the licence-doc was removed by this commit cfaa072 , maybe we can remove the "React documentation is Creative Commons licensed." paragraph because of the migration of the document website.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 8, 2017

The documentation and source code for reactjs.org now lives in a different repository: reactjs/reactjs.org. (For more info on why we made this move, see issue #11075.)

I've moved this issue to the new repo: reactjs/react.dev#84

Let's continue the discussion there!

@bvaughn bvaughn closed this as completed Oct 8, 2017
@sag1v
Copy link

sag1v commented May 23, 2019

@bvaughn The reactjs/reactjs.org link you posted is broken i think you meant https:/reactjs/reactjs.org/ ?

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

No branches or pull requests