Refactor Anontools #4845
Refactor Anontools #4845
Conversation
✅ Deploy Preview for volto canceled.
|
Passing run #5731 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
|
@Tishasoumya-02 @nileshgulia1 @JeffersonBledsoe @jackahl I've refactored the |
|
When destructing the return value of useContent ,unit tests were failing so reverted it back . I guess the prop Types are doing the work for it 🤔 |
| import { useSelector, shallowEqual } from 'react-redux'; | ||
|
|
||
| export function useContent() { | ||
| const data = useSelector((state) => state.content.data, shallowEqual); |
There was a problem hiding this comment.
@tiberiuichim what do you think of this?
There was a problem hiding this comment.
@sneridagh I'm not sure, in principle is fine, but I have some concerns.
I'm genuinely curious about all the "ceremony", with multiple hook calls and shallowEqual. Why not just return state.content?
const content = useSelector((state) => state.content);
return {
data: content.data,
loading: content.get.loading,
...
}
I think that at this point, while we wait for useQuery, the hook is underpowered. It can't be used for content subrequests, which highlights the other problem: the hook right now only takes care of yielding content info, it doesn't trigger a getContent action.
Following this train of thought, I think it's better to either go full useQuery or not at all. If we introduce this, we'll have multiple variations of this type of code in the codebase (old style, useContent, etc), which will all have to be migrated to the new variation of useContent.
The counter-argument is that the useContent with subrequests is not needed in vanilla Volto, so it's fine to introduce it right now as it is and enhance it with params and options later.
There was a problem hiding this comment.
Thanks for the review! Let's try to talk today!
|
Congrats! @Tishasoumya-02 The first one! 🎉 Let's continue for the rest! |
* master: Release 17.0.0-alpha.14 Linked headlines (#3540) Release notes for 16.20.8 16.21.0 16.21.1 (#4910) Spanish translation (#4896) Refactor Anontools (#4845) Update to plone-backend 6.0.5 (#4897) Release 17.0.0-alpha.13 Enforce max upload size (#4868) Fix and improve the `addStyling` helper (#4880) Release 17.0.0-alpha.12 Fix regression in horizontal scroll in contents view, add it back (#4872) Configurable Container component from registry for some key route views. (#4871) Allow to deselect color in ColorPickerWidget. (#4839) Release 17.0.0-alpha.11 Pagination with router params (#4698) Release 17.0.0-alpha.10 feat(slate): Add css identifier to slate style menu options (#4847) Update Brazilian Portuguese translations (Fixes #4853) Convert header class to function (#4767)
* master: (29 commits) Remove anonymous function calls. Remove default exports from. (#4917) Release 17.0.0-alpha.14 Linked headlines (#3540) Release notes for 16.20.8 16.21.0 16.21.1 (#4910) Spanish translation (#4896) Refactor Anontools (#4845) Update to plone-backend 6.0.5 (#4897) Release 17.0.0-alpha.13 Enforce max upload size (#4868) Fix and improve the `addStyling` helper (#4880) Release 17.0.0-alpha.12 Fix regression in horizontal scroll in contents view, add it back (#4872) Configurable Container component from registry for some key route views. (#4871) Allow to deselect color in ColorPickerWidget. (#4839) Release 17.0.0-alpha.11 Pagination with router params (#4698) Release 17.0.0-alpha.10 feat(slate): Add css identifier to slate style menu options (#4847) Update Brazilian Portuguese translations (Fixes #4853) Convert header class to function (#4767) ...
#4460