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] Use hooks in controllers #3213

Merged
merged 6 commits into from May 13, 2019
Merged

[RFR] Use hooks in controllers #3213

merged 6 commits into from May 13, 2019

Conversation

djhi
Copy link
Contributor

@djhi djhi commented May 11, 2019

I believe the first step before migrating our controller components to custom hooks is to make them use hooks in the first place.

  • CreateController
  • EditController
  • ShowController
  • ListController
  • Unit test the utility functions which have been extracted

@djhi djhi added this to the 3.0.0 milestone May 11, 2019
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Super encouraging. I'm amazed at how compact and expressive the code looks when we use hooks.

isLoading: boolean;
translate: Translate;
}
export const getRecord = ({ state, search }, record: any = {}) =>
Copy link
Member

Choose a reason for hiding this comment

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

this should live in the reducers directory as it's a selector (same with the test of this function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will extract selectors

location: { state, search },
record,
} = this.props;
this.record =
Copy link
Member

Choose a reason for hiding this comment

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

I think you've broken the clone feature by not porting this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The e2e tests disagree with you ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And btw I ported this. This is the getRecord function you commented earlier

version,
} = this.props;
useEffect(() => {
dispatch(resetForm(REDUX_FORM_NAME));
Copy link
Member

Choose a reason for hiding this comment

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

resetForm wasn't called on mount in the previous version. Isn't there a consequence of calling it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have e2e tests for this case but I will check

@kopax
Copy link
Contributor

kopax commented May 13, 2019

This looks cool thanks! Will we have a part about what are controllers in the documentation (RFC style)?

@djhi
Copy link
Contributor Author

djhi commented May 13, 2019

Will we have a part about what are controllers in the documentation (RFC style)?

See the roadmap 😉 (#3194)

@djhi djhi changed the title [WIP] Use hooks in controllers [RFR] Use hooks in controllers May 13, 2019
@fzaninotto fzaninotto merged commit 6ee4f23 into next May 13, 2019
@fzaninotto fzaninotto deleted the use-hooks branch May 13, 2019 15:53
@fzaninotto
Copy link
Member

Awesome!

@fzaninotto fzaninotto mentioned this pull request May 13, 2019
40 tasks
@djhi
Copy link
Contributor Author

djhi commented May 13, 2019

Awesome!

🤗

Now we can extract custom hooks 🚀 !

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.

None yet

3 participants