Skip to content

Conversation

@timburgess
Copy link

Very cool little project.

i think the BasicExample could be improved by:

  • being explicit about the fact that the css is required

  • Seeing React beginners as the audience, I think it's unwise to use a constructor for just creating state and binding handlers. It seems to become a pattern for a lot of people just copy/pasting code when in fact the constructor is not required.

@jakezatecky
Copy link
Owner

Hello and thanks for the PR! My (belated and long) response is as follows:

On CSS requirement: I have the usage of CSS being quite prominent in the README, being the very next header after installing the package. The relatively simple example page also loads a CSS file, although it is not immediately clear that it contains the styles included with this library. I make reference to the fact that the developer can run an import on the CSS in the README, provided that they are using a CSS loader, but I generally dislike that method as it provides limited ability to customize any of the styles. I still provide the JS import example as as a courtesy to any developer using that method. You might have noticed that the Travis CI build failed because I am not using the CSS loader anywhere in the examples, although that could be easily added.

I would argue that using a CSS loader is a bit more complicated than a CSS import because that means the file is loaded in JavaScript as opposed to the more universal <link /> method in HTML, the method that every new web developer would presumably learn about. Still, I am toying with the idea of adding commented out code that includes this import statement with an explainer comment above (though I am generally extremely averse to ever having any commented-out code).

On the medieval constructor: I am more than open to moving the initial state into the class properties. When I initially wrote those examples, class property support was still very poor for the linting tool I was using, and it was annoyingly trying to have non-methods arrange in-between methods. I will consider that for a future revision.

I am less sold on using the arrow bind on methods, as I never really liked that method of dealing with event handler binding. Ideally, classes would automatically bind all methods to have a lexical this, but for whatever inexplicable reason, that was never included in the spec for ES6. The arrow bind method, while being a bit less verbose (something I generally am all-for), rattles me the wrong way due to its divergence of the method signature compared to other methods. It is also easy to overlook this binding at a glance. With constructor binds, it is much harder to overlook which methods are being bound in such a way and all methods have the same general signature. I have toyed with using one of the various autobinders, but have decided against that for the time being.

Overall, I appreciate your PR and insights, but I think I will only move forward with the ES7 class property for the state variable.

jakezatecky added a commit that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants