ES6 all React stuff.

[rstudner]

I also cleaned up all the trailing commas (that where everywhere) because they are lint etc issues (rubymine in particular really dislikes them). They hurt nothing to take out.

@justin808 After submitting a comment, which works, there is still an issue (that seems like) a "this.props" where this isn't defined.

You'll notice in the PR various uses of .bind(this).. that is because ES6 react classes don't automagically do that anymore (so like, a clickHandler method cannot refer to "this" (which they all do) unless you do this._handleClick.bind(this) sort of syntax)

Clearly I missed one/haven't unraveled it yet :) That being said, this has to be 95-99% there and this way more eyes can get on it.

[justin808]

@rstudner Thanks so much! ES6 is great. However, we're trying to keep with the AirBnb JS style guideline as much as possible. The trailing commas are there for a reason.

For the bind parts, we want to use the ES7 :: syntax.

@alexfedoseev anything else?

[justin808]

Connects to #139

[rstudner]

Ahh! my apologies on their removal then! :) I can go put them back in easily enough. (And of course i'm now off reading the AirBNB style guide so I don't do this again etc)

Sorry, I didn't think to 'go ES7' (despite of course the Readme.MD saying ES7 ;). I was mostly just trying to mimic syntax people would see if they were reading the official React docs and weren't familiar with ES7.

[robwise]

I think it's better to avoid using constructor in React components, especially since we aren't even setting state here. If we do need to set state, it should be via class a class property. For example, if we want to have a state of `{author: "Timmy", text: "hello world"}, you just do this:

class Comment extends React.Component {
  author = "Timmy";
  text = "hello world";
....

[justin808]

@rstudner the only CI failure is linting.

@alexfedoseev maybe comment if we should go with this?

[alex35mil]

@justin808 @rstudner This commit: shakacode/react-editable-grid-example@f7968e1

There are 2 branches:

• master (with functions, working)
• fat-arrows (with fat-arrows functions, broken)

Please confirm, that second one is broken.

[rstudner]

Pushed an update to make the lint-robot happy :)

In regards to fat arrow vs :: and other syntax things, the longest(?) explanation I found for them was here:
http://egorsmirnov.me/2015/08/16/react-and-es6-part3.html

[alex35mil]

The rule is simple: fat arrows works without binding only within one component. And it's broken when they are passed to outer world. When fat arrow functions are used, we have to keep track of the cases when we have to bind function to context and when it's not necessary. So the question is: do we need one more thing to remember?

[alex35mil]

All in all, I vote for binding methods to context in constructor. One more reason, btw.

[rstudner]

@alexfedoseev - I 100% agree with binding methods in the constructor. Earlier in this (long hah) PR of comments I was told to not use constructors.

Thus -- I went with what I thought was the 'next best' option.

As well, in a Redux world I assumed we wouldn't be doing the 'passing of callback functions down via props' and thus none of these would leak outside of their component of origination.

[alex35mil]

As well, in a Redux world I assumed we wouldn't be doing the 'passing of callback functions down via props' and thus none of these would leak outside of their component of origination.

@rstudner It happens quite often, for example when we need to pass the callback to save / close reusable modal dialog.

[rstudner]

@alexfedoseev Ahh, i've been handling it differently just on my own personal projects -- but yeah, that would be a widely encountered use case for sure.

Based on your own blog post, which syntax would you go for (you are using .bind(this) in the constructors and as well, earlier in this PR, I was told not to use .bind(this)).

[alex35mil]

Let's stick with:

constructor(props, context) {
  super(props, context);

  this._handleSomething = this._handleSomething.bind(this);
}

It's less problematic way of doing binding.

We can do something like:

// bindToContext.js
export default methods => {
  methods.forEach(method => this[method] = this[method].bind(this))
}

// Component
constructor(props, context) {
  super(props, context);

  bindToContext([ '_handleSomething' ]).bind(this);
}

@jbhatab @justin808 @robwise @rstudner

[rstudner]

Had a good quick chat with Alex over Slack. Will make some updates in a couple hours (have to feed small childrenBeasts and get them on the bus)

[justin808]

@rstudner Let's squash and create a nice commit message that sums up the choices. And then we'll merge.

[justin808]

one last change now that we're using lodash:

_.bindAll(this, '_ajaxCounter', '_isSendingAjax');

[rstudner]

Thoughts on the above? I can import lodash in each file, or we can expose it. the above commit just exposes it like react/jquery.

Not sure if this has to be done in the server file (we expose React, but not jquery for example there)

[alex35mil]

We should expose something only for the reason. For example, React should be exposed for the gem or fetch should be exposed since it's (probably) future global object. There is no reason to expose lodash, so it should be imported.

[rstudner]

Okay, i'll change to imports. Interestingly, exposing it solved 99% of the errors, but not all of them (which is odd all by itself)