Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Update Overview.md #841

Merged
merged 1 commit into from
Dec 11, 2016
Merged

Update Overview.md #841

merged 1 commit into from
Dec 11, 2016

Conversation

oscarmorrison
Copy link
Contributor

Before submitting a pull request, please make sure the following is done...

  1. Fork the repo and create your branch from master.
  2. If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure that:
  • The test suite passes (npm test)
  • Your code lints (npm run lint) and passes Flow (npm run flow)
  • You have followed the testing guidelines
  1. If you haven't already, complete the CLA.

Please use the simple form below as a guideline for describing your pull request.

Thanks for contributing to Draft.js!

Summary
Remove unneeded code in example
[...]

Test Plan
No functional change
[...]

Simplifies code and minimizes new concepts.

@oscarmorrison
Copy link
Contributor Author

@davidchang @hellendag 👀

@flarnie
Copy link
Contributor

flarnie commented Dec 10, 2016

Hi @oscarmorrison! We appreciate your attention to the documentation and helping us improve. I believe that the proposed change would put that line over 80 characters, which is the limit we usually use for line length within Facebook. I assume that is why the variable assignment was there.

In fact - if you're looking for quick wins to improve this project, fixing long lines in the Draft.js source and docs would be helpful. There are many lines over 80 characters throughout the Draft.js source. We're looking to enable better linting of this, but for now I don't think our linter is flagging it.

@flarnie flarnie closed this Dec 10, 2016
@oscarmorrison
Copy link
Contributor Author

Hi @flarnie thanks for the comment.
The line is now exactly 80 characters.

I was actually doing it as I was ramping someone up on Draft who was also new to React (& ES6)
and they found the destructuring confusing.

Also as you can see in the example code on the next page in the docs:
https://facebook.github.io/draft-js/docs/quickstart-api-basics.html#content
return <input value={this.state.value} onChange={this.onChange} />;

is there. so it keeps the consistency

@flarnie flarnie reopened this Dec 11, 2016
@flarnie
Copy link
Contributor

flarnie commented Dec 11, 2016

@oscarmorrison You have a good point about the destructuring potentially being confusing for folks who are new to ES6/JS/React, and the new syntax is also more consistent with the other example.

The line is exactly 80 characters when we don't take the indentation into account. With the indentation it crosses the 80 character line, shown in red in my editor here:
screen shot 2016-12-10 at 4 53 33 pm

In cases like this we usually use parenthesis to wrap to the next line:
screen shot 2016-12-10 at 4 57 10 pm

I'd be happy to land this if it were tweaked in this way, and thanks for explaining. I should have made this suggestion in the first place rather than closing the PR, sorry about that.

@oscarmorrison
Copy link
Contributor Author

Done. Thanks for reopening.

@oscarmorrison
Copy link
Contributor Author

Also it seems that your linter max len is 120:
https://github.com/facebook/fbjs/blob/master/packages/eslint-config-fbjs/index.js#L310

Simplifies code and minimizes new concepts.
@oscarmorrison
Copy link
Contributor Author

Fixed a spacing typo and rebased. All good to go. Thanks @flarnie

@flarnie
Copy link
Contributor

flarnie commented Dec 11, 2016

Thanks for flagging that the linter is still set to 120. Internally we just moved to 80 fairly recently, I need to get the linter here in sync with our internal one. And thanks for updating this!

@flarnie flarnie merged commit 2de3c45 into facebookarchive:master Dec 11, 2016
@oscarmorrison oscarmorrison deleted the patch-1 branch December 14, 2016 18:39
ouchxp pushed a commit to ouchxp/draft-js that referenced this pull request Apr 7, 2017
Simplifies code and minimizes new concepts.
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.

3 participants