silverstripe-asset-admin
silverstripe-asset-admin copied to clipboard
Refactor <Gallery> and <Editor> into containers and presentational components
Currently, these components have both state and render views, which is an antipattern - see https://medium.com/@learnreact/container-components-c0e67432e005#.jv7qgdyhi and https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.b5tp6c91z.
This is setting a bad precedent on our own conventions (we have separate containers
and components
folders, but they're all over the show)
Note that kickassets does this pretty well, check containers
vs. views
folders: https://github.com/unclecheese/silverstripe-kickassets/tree/master/javascript/src/
@flamerohr @unclecheese Do you think this is the next most important cleanup activity to work on after Chris' work on "Cleanup React Conventions" (https://github.com/silverstripe/silverstripe-framework/issues/6427)? What React code patterns are giving us the most grief in practice? I'm keen to sort out containers vs. components so we can create a pattern library that's not dependant on Redux/GraphQL and other plumbing, but that's broader (and more shallow) than this cleanup task.
Yes, I think so :) At the moment, it's very difficult to shift presentation elements around in these Components without the need to also jump through the redux state. Also if we want to allow these components to become customisable (Injector), it's much easier to customise a presentation component when it doesn't handle a state.
Probably outside the scope of this work: We could also look at how we're communicating across components when something needs to refresh, which sometimes need weird workarounds to achieve... e.g. to refresh the Editor, you need to close then reopen the Editor and that causes the gallery to reload twice as well.
Agreed. My biggest concerns are:
- More widespread use of Injector
- More reliance on redux for actions instead of passing handler props
- More reliance on redux for state instead of passing data props (injecting the store deeper in the tree improves rendering performance, too)
- Make components more single-concern
- More containers, as described above
We could also look at how we're communicating across components when something needs to refresh
This would be solved by more widespread use of GraphQL, right? The fact that form submissions aren't using GraphQL seems to be at the root of this issue.
@unclecheese Can you please write up a ticket or two about your proposed improvements? Something that'll fit in a ~2 day timebox.
Sigh, not going to fit in the milestone