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

[RFR] Migrate simple example to hooks #3225

Merged
merged 15 commits into from
Jun 4, 2019
Merged

Conversation

zyhou
Copy link
Contributor

@zyhou zyhou commented May 15, 2019

  • Migrate PostList.js hooks
  • Migrate PostCreate.js hooks
  • Migrate ResetViewsButton.js hooks
  • Migrate CommentEdit.js to hooks
  • Migrate CommentList.js to hooks
  • Migrate PostPreview.js to hooks
  • Migrate PostQuickCreate.jshooks
  • Migrate PostQuickCreateCancelButton.js to hooks
  • Migrate PostReferenceInput.js to hooks
  • Migrate UserEdit.js hooks
  • useSelector isLoading (don't remove the props)

Review with "Hide whitespace changes" params

@zyhou zyhou force-pushed the migrate-example-simple-hooks branch from 621d571 to 06d8d8c Compare May 15, 2019 09:55
@zyhou zyhou changed the title [WIP] Migrate simple example to hooks [WIP] Migrate simple example "posts" ressource to hooks May 15, 2019
examples/simple/src/posts/PostCreate.js Outdated Show resolved Hide resolved
examples/simple/src/posts/ResetViewsButton.js Outdated Show resolved Hide resolved
@zyhou zyhou changed the title [WIP] Migrate simple example "posts" ressource to hooks [WIP] Migrate simple example to hooks May 15, 2019
@djhi djhi added this to the 3.0.0 milestone May 16, 2019
@zyhou zyhou marked this pull request as ready for review May 23, 2019 20:45
@zyhou zyhou force-pushed the migrate-example-simple-hooks branch 2 times, most recently from 9c8f2d7 to 4f2d985 Compare May 23, 2019 20:57
@zyhou
Copy link
Contributor Author

zyhou commented May 24, 2019

@djhi Can you take a look on PostQuickCreate.js I don't understand why I can't use useSelector in this case.

If you use useSelector, we need to click twice on "save" button

@zyhou zyhou requested a review from djhi May 24, 2019 08:37
@zyhou zyhou changed the title [WIP] Migrate simple example to hooks [RFR] Migrate simple example to hooks May 24, 2019
@fzaninotto fzaninotto force-pushed the migrate-example-simple-hooks branch from 464e020 to 9b130d0 Compare May 24, 2019 15:34
@fzaninotto
Copy link
Member

@zyhou I fixed the useSelector bug

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed today, I think we shouldn't use useCallback all the time but only if and when we notice performance issues. Not sure it is important though.

@fzaninotto
Copy link
Member

No, useCallback wasn't the cause of the problem.

@djhi
Copy link
Collaborator

djhi commented May 24, 2019

No, useCallback wasn't the cause of the problem.

I didn't say it was. I just think they aren't necessary unless a performance issue arise.

@zyhou zyhou requested review from djhi and fzaninotto and removed request for djhi May 26, 2019 21:53
@fzaninotto fzaninotto requested a review from djhi May 27, 2019 12:44
@djhi
Copy link
Collaborator

djhi commented May 27, 2019

I really think we are over-complicating things with those useCallback. They shouldn't be the default and it probably shouldn't be what we show in an example.

@fzaninotto
Copy link
Member

Let's merge it like that. We'll review it once the core performance gets on par with 2.0.

@fzaninotto fzaninotto merged commit f389aa6 into next Jun 4, 2019
@djhi djhi deleted the migrate-example-simple-hooks branch June 4, 2019 07:04
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.

3 participants