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] Convert GET_MANY accumulate saga to hooks #3550

Merged
merged 15 commits into from
Aug 22, 2019
Merged

[RFR] Convert GET_MANY accumulate saga to hooks #3550

merged 15 commits into from
Aug 22, 2019

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Aug 20, 2019

Using a hook in ReferenceField allows to handle corner cases that were impossible to deal with using sagas. Most notably, ReferenceField now works properly when the dataProvider fails or returns nothing for the GET_MANY call.

This is mostly a refactoring with few visible changes for the end user, labeled 'NEW' below.

  • Create useGetMany hook based on crudGetManyAccumulate action logic
  • Add unit test for useGetMany
  • Use the new hook in useReference
  • NEW Hide loader in <ReferenceField> if the dataProvider call fails or returns nothing
  • NEW Display an error icon in <ReferenceField> if the dataProvider fails
  • Use the new hook in useReferenceArrayFieldController
  • Use the new hook in ReferenceArrayInputController (too long, to be done in another PR)
  • Fix tests

Closes #3552, #3451, #2388, #854.

To be done in another PR:

  • Create useGetMatching hook based on crudGetMatchingAccumulate action logic
  • Add unit test for useGetMatching
  • Use the new hook in ReferenceInput, ReferenceArrayInput, SelectArrayInput and SelectInput
  • Deprecate accumulate saga

This change is backwards compatible with the accumulate saga, i.e. dispatching crudGetManyAccumulate actions still works with the saga. The new useGetMany hook does not dispatch such actions.

@fzaninotto fzaninotto added this to the 3.0.0 milestone Aug 20, 2019
@fzaninotto fzaninotto changed the title [WIP] Convert accumulate saga to hooks [WIP] Convert GET_MANY accumulate saga to hooks Aug 21, 2019
@fzaninotto fzaninotto changed the title [WIP] Convert GET_MANY accumulate saga to hooks [RFR] Convert GET_MANY accumulate saga to hooks Aug 21, 2019
@fzaninotto
Copy link
Member Author

Switching to RFR

@fzaninotto
Copy link
Member Author

oops, sorry, I missed a side effect, back to WIP

@fzaninotto fzaninotto changed the title [RFR] Convert GET_MANY accumulate saga to hooks [WIP] Convert GET_MANY accumulate saga to hooks Aug 21, 2019
@fzaninotto fzaninotto changed the title [WIP] Convert GET_MANY accumulate saga to hooks [RFR] Convert GET_MANY accumulate saga to hooks Aug 21, 2019
@fzaninotto
Copy link
Member Author

Well, that's in ReferenceArrayInputController, so not in this PR. Back to RFR.

@@ -144,7 +144,11 @@ export const ReferenceFieldView = ({
}
if (error) {
return (
<Error aria-errormessage="Error" color="error" fontSize="small" />
<Error
aria-errormessage={error.message ? error.message : error}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we translate this error ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, yes. In a future PR? I'd like to have this pr merged quickly to release a new alpha soon.

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.

Awesome!

@djhi djhi merged commit d004b3d into next Aug 22, 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