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

Refactor <Gallery> and <Editor> into containers and presentational components #179

Open
chillu opened this issue May 31, 2016 · 5 comments

Comments

@chillu
Copy link
Member

chillu commented May 31, 2016

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/

@chillu
Copy link
Member Author

chillu commented Sep 10, 2017

@flamerohr @unclecheese Do you think this is the next most important cleanup activity to work on after Chris' work on "Cleanup React Conventions" (silverstripe/silverstripe-framework#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.

@flamerohr
Copy link
Contributor

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.

@unclecheese
Copy link

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

@chillu
Copy link
Member Author

chillu commented Sep 11, 2017

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.

@chillu
Copy link
Member Author

chillu commented Sep 20, 2017

Sigh, not going to fit in the milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants