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

Refactoring containers/components/proptypes #3003

Closed
jroebu14 opened this issue Aug 8, 2019 · 1 comment
Closed

Refactoring containers/components/proptypes #3003

jroebu14 opened this issue Aug 8, 2019 · 1 comment
Labels
investigation question Further information is requested technical-work Technical debt, support work and building new technical tools and features
Milestone

Comments

@jroebu14
Copy link
Contributor

jroebu14 commented Aug 8, 2019

In Simorgh I think the separation of containers, components and prop types is becoming hard to manage and understand because they are fragmented across the project and sort of out sight and out of mind from one another.

I also think our container/presentation component pattern implementation is blurry and it's not a recommended pattern any more as stated here https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0. Instead we could take a different approach as discussed here kentcdodds/ama#545 and elaborated on here https://www.youtube.com/watch?v=l6GTpKLWllQ&list=PLV5CVI1eNcJgCrPH_e6d57KRUTiDZgs0u but I think that most of (if not all) of our containers are just regular components.

My idea would be to just have a components directory and do with away with the container/presentation pattern. For any data fetching and handling we could create a custom hook containing any stateful logic and side effects which is kept separate from the presentational component. Compare this to container components which requires the creation/rendering of two separate components.

Also we should move any centralised prop type definitions to the appropriate components. If a parent component needs to share a child's prop types then the parent component can just import the child component's prop types like so.

import Promo from './Promo';

const PromoList = promos => promos.map(promo => <Promo {...promo} />);

PromoList.propTypes = {
  promos: arrayOf(Promo.propTypes),
};

export default PromoList

On a side note I also think prop type definitions should be explicit and not generated/abstracted away because it can be extremely useful to the consumer of the component to not have to go digging around as to what the component expects.

This is just to have a discussion so please tell me if you disagree!

If we think this is a good approach then I can pick a container/component to refactor and demonstrate this idea.

@dr3 dr3 added investigation question Further information is requested labels Aug 8, 2019
@jamesbhobbs jamesbhobbs added this to the Simorgh 3.0 milestone Aug 20, 2019
@jroebu14 jroebu14 added the technical-work Technical debt, support work and building new technical tools and features label Jul 6, 2020
@andrewscfc
Copy link
Contributor

Duplicate of: #4478 and #4477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigation question Further information is requested technical-work Technical debt, support work and building new technical tools and features
Projects
None yet
Development

No branches or pull requests

5 participants