vrk / weeknotes

A web app that keeps track of your weekly notes
https://weeknotes.herokuapp.com/
0 stars 1 forks source link

Figure out a consistent set of rules for storing Component state #39

Open vrk opened 8 years ago

vrk commented 8 years ago

WeekNoteEditor is a good example of where I'm inconsistent on this: https://github.com/vrk/weeknotes/blob/master/client/src/components/editor/WeekNoteEditor.js

So for onFocus, I'm modifying the DOM directly:

  _onFocus(e: Event) {
    assert(e.target instanceof HTMLTextAreaElement);
    const element = e.target;
    if (element instanceof HTMLTextAreaElement) {
      element.setSelectionRange(0,0);
      element.scrollTop = 0;
    }
  }

... that seems non-React-ish, in that I'm directly setting properties on element instead of maintaining a copy of the state in this.state and updating the <textarea> in the render() function.

I.e. ideally I feel like the "react" way of doing things would be:

<!-- hypothetical, non-working code: -->
<textarea selectionStart={this.state.selectionStart} selectionEnd={this.state.selectionEnd} />

But I don't think it's possible in this case to update the selectionRange via HTML; I believe it must be called in JavaScript (jsbin 1; jsbin 2; MDN)

I need to develop some rules about when to store state where and how.

State can be in the following locations:

When is each one appropriate?

awong-dev commented 8 years ago

So 2 things: (1) React Elements are Virtual DOM elements and not HTML Elements. So I don't think the html update rules apply. In particular, if you change an attribute in a React Virtual DOM element, it should cause React to reconcile the underlying HTML element...thus I would test whether or not changing the setSelectionStart attribute in the React TextArea dom will change things.

(2) Redux state (all state should be global state IMO since the idea is to have 1 store per app) should be content or anything related to the app's model...if you navigate back to a page and want to be able to recover that state, then it should be in Redux. Overall, I would generally bias towards keeping everything in Redux.

For this code in particular, I don't see a need for a private method for onFocus. Also, the onX prefix is by convention the name of the attribute, but the handler function should be called handleX so _onFocus should be _handleFocus or handleFocus.

I also don't see a need for the type-check at 34. Your debug assert is good enough assuming you have test coverage? There's no other way to access that function, so....?

Lastly, since that function doesn't actually use any state off of this, maybe just make it a module function (similar to a C++ static)? You could drop the bind() then.