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

Feature/cleanup react components #282

Merged
merged 3 commits into from
Feb 18, 2016
Merged

Feature/cleanup react components #282

merged 3 commits into from
Feb 18, 2016

Conversation

lifeiscontent
Copy link
Contributor

@justin808 I updated all the react components in the dummy app to use the new Stateless Functional Components signature

as well as upgrade the new react-router signature

Review on Reviewable

web-console should only be added during development
* Update to new Signature using RouterContext
* Update to new history signature using browserHistory
* now using new Stateless functional components signature
@justin808
Copy link
Member

EXCELLENT. One main concern is that we have at least one registered component does the extend React.Component, as we need to make sure the internals of ReactOnRails can handle that.


Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 27 [r1] (raw file):
@alexfedoseev I guess this is the new react-redux API. Way more readable!

CC: @robwise


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 31 [r1] (raw file):
We need to be sure that we have examples of:

  • React.createClass
  • extends React.Component
  • pure render function

So yes, this could be written as a pure render function, but we're losing test fidelity.

BTW, this code looks great!


spec/dummy/client/app/components/RouterSecondPage.jsx, line 12 [r1] (raw file):
We could just have the export default above and inline the variable.

@robwise @alexfedoseev ?


Comments from the review on Reviewable.io

@lifeiscontent
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


spec/dummy/client/app/components/RouterSecondPage.jsx, line 12 [r1] (raw file):
@justin808 I generally like to name things because it allows for less friction when you need to add something like PropTypes to a component.


Comments from the review on Reviewable.io

@lifeiscontent
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 31 [r1] (raw file):
@justin808 sounds good, just let me know what I should change and I'll do that! 😄


Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

Review status: all files reviewed at latest revision, 3 unresolved discussions.


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 27 [r1] (raw file):
Looks all the same for me, but w/o decorators (look in fng properties container), what actually changed?

BTW decorators waaaay more readable when you need to apply more than one HOC.


spec/dummy/client/app/components/RouterSecondPage.jsx, line 12 [r1] (raw file):
Currently we use class way to define React components, and add propTypes via statics, b/c pure functions have limited functionality, so usually we export default the named class.


Comments from the review on Reviewable.io

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 3 unresolved discussions.


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 27 [r1] (raw file):
@alexfedoseev HOC?

@aaronvb Is this from the latest docs?


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 31 [r1] (raw file):
You probably should just make this one extend the 


Comments from the review on Reviewable.io

@justin808
Copy link
Member

Review status: all files reviewed at latest revision, 4 unresolved discussions.


spec/dummy/client/app/components/HelloWorldWithLogAndThrow.jsx, line 5 [r1] (raw file):
We've got one component that meets the "have one component" so we're all set.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Feb 18, 2016
@justin808 justin808 merged commit ce514f0 into shakacode:master Feb 18, 2016
@justin808
Copy link
Member

Thanks @lifeiscontent. Nice Job!

👏

@alex35mil
Copy link
Member

Review status: all files reviewed at latest revision, 4 unresolved discussions.


spec/dummy/client/app/components/HelloWorldContainer.jsx, line 27 [r1] (raw file):
Higher-order Components. Here is an example w/ just 2 HOCs:

// w/ decorators
@doOneThing(withParams)
@doAnotherThing(withAnotherParams)
class Foo { ... }

// w/o decorators
doOneThing(withParams)(doAnotherThing(withAnotherParams)(Foo))

Comments from the review on Reviewable.io

@lifeiscontent
Copy link
Contributor Author

@alexfedoseev aren't decorators deprecated? That's what I think I've read.

@alex35mil
Copy link
Member

@lifeiscontent They aren't deprecated, decorator's draft is being reworked and this is why they temporarily removed from Babel. So they're not going anywhere, at least I hope.


Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

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.

4 participants