Restructure folders - Githubissues

tgrrr commented 4 years ago

Here's the folder structure that I'm suggesting to implement:

cypress/
  integrations/
    components/
      ComponentName.spec.js <- BDD spec
      ComponentName/
        ComponentName.spec.js <- Cypress/Jest spec
src/
  assets/
    images/
    ...

  common/
    ErrorBoundary/
    utils/
      util1.js
      util2.js
      index.js
    ui/
      Input/
      Form/
      Checkbox/

  config/
    routes.js

  components/
    App/
    Modal/
    ...

  pages/
    HomePage/
    LoginPage/
    PageSections/
      Header/
      Footer/
      Main/
    Routes/
    Template/

  store/ <- option #1 to be fleshed out

Each Component/Page would have the structure:

ComponentName/
  ComponentNameContainer.js
  ComponentName.js
  ComponentNameStyled.js
  ComponentStore.js <- option #2 to be fleshed out
  index.js

Cheers,

Phil

lpatmo commented 4 years ago

Thank you for this sketch, @tgrrr! :) I have a couple of questions, if you don't mind helping me broaden my understanding:

1/

ui/
      Input/
      Form/
      Checkbox/

Do we need this folder if our components are mostly going to be imported from from "@material-ui/core";?

2/ What is "store"? store/ <- option #1 to be fleshed out

3/ Why do we need both a ComponentNameContainer.js and an index.js?

4/ (Unrelated) What are your thoughts on using https://github.com/testing-library/react-testing-library for writing tests?

tgrrr commented 4 years ago

Do we need this folder if our components are mostly going to be imported from from "@material-ui/core";?

I'd say yes.

Firstly, because it's DRY-er.

Also if:

We have a bug in our Button component
We upgrade a component. Eg. We add icons to our buttons
We changed the very excellent material-ui library for another
The requirements change
We want to add a test wrapper <div data-testid='foo'> or style wrapper <div className='bar'}> to our component

...then we have to fix it in one place rather than the 50 place

The component can be as simple as:

// Button.js
import React from 'react';
import { Button as MaterialButton } from '@material-ui/core';

const Button = props => <Button ...props>{children}</Button>

It also means we can setup our regular components to import in the following way:

import { Button, Input, Switch } from 'ui';

2/ What is "store"? store/ <- option #1 to be fleshed out

Are we going to use Redux/similar?
If so, should we keep that store in a /store folder, or with each page?

3/ Why do we need both a ComponentNameContainer.js and an index.js?

The container is for non-presentational logic. Eg. State.
The index.js is helpful with imports. Say we don't initially have a container, and then add one.

Without index.js a git update looks like this, and could potentially be across thousands of files

- import { Button } from '../ui/Button/Button'
+ import { ButtonContainer } from '../ui/Button/ButtonContainer'

...

- <Button />
+<ButtonContainer />

- <Button />
+<ButtonContainer />

- <Button />
+<ButtonContainer />

With index.js, the git commit looks like this:

// ui/index.js
- import { Button } from 'Button'
+ import { ButtonContainer as Button } from 'ButtonContainer'

export { Button }

4/ (Unrelated) What are your thoughts on using https://github.com/testing-library/react-testing-library for writing tests?

I'm a really, really, really big fan of react-testing-library inside of Cypress (using BDD). See: Cypress Testing Library

For reference: I tested a React app with Jest, Enzyme, Testing Library and Cypress. Here are the differences.

lpatmo commented 4 years ago

1/ Okay, I am 😍. That makes sense, thanks for the example!

2/ > Are we going to use Redux/similar? Ah, understood now. Hmm, maybe this warrants a larger/separate discussion, but leaning towards "not unless we obviously need it." I'm not convinced we do yet.

3/ Hmm, so:

Button/
  ButtonContainer.js
  Button.js
  ButtonStyled.js
  ButtonStore.js (if relevant)
  index.js

I'm not used to seeing so many files for one component, but maybe it'll become clearer after I see an example. (I'm used to Button/ -> index.js + ButtonStyled.js, basically)

4/ Yay!

tgrrr commented 4 years ago

1/ & 3/ I was having a look at the other issues, and the folder structure would also help with Storybook which is @sebbel 's suggestion here https://github.com/codebuddies/react-concept/issues/71

2/ I don't think Redux is necessary yet either

3/ It's similar to your Button/ -> index.js + ButtonStyled.js with the addition of Container components. See: Presentational and Container Components by Dan Abramov Also, by keeping the Presentational component in ComponentName, it means that we don't end up with 16 index.js files in our IDE's like vscode or Atom

jesperhodge commented 4 years ago

I haven't worked with Redux yet, but it seems to add a lot of complexity and even for huge production apps I don't see many benefits any longer (we have context for state management - sure, that can get a bit unorganized, but only if the code structure we build is not so good.)

I absolutely suggest also to split presentational and container components. That can help immensely to keep code organized, which is crucial here. Abramov's comment that he does not recommend this anymore seems very very high-level - he still suggests separating business and UI logic, only using hooks instead of containers - that's more up to us, but I suggest still separating both parts for now.

Then - I think @lpatmo you underestimate a bit how much we have to customize components, even if we use Material UI completely as it is and don't add fancy stuff, so I also agree with @tgrrr strongly.

I love the cypress integration already planned out, and only suggest we do the same for javascript unit tests already, and just add that from the start as a practice we want.

It could be helpful to just plan out a rough data flow structure using contexts as long as we are not using Redux. One thing I suggest from the start is to mostly avoid using Higher-Order-Components for contexts. I have extremely bad experience with that. We can just use normal providers and useContext hooks.

Also it might make sense deciding how our React components should look - I like that you are using functions and hooks, and my experience is a hundred times better than using class components, I basically never use classes any more and that solved many problems.

jesperhodge commented 4 years ago

So as for the proposed folder structure, I really like it and as it is so important to organize React code well - playing a big factor in the success or failure of projects even - I strongly recommend either using the proposed structure, or coming up with something similar at least.

sebbel commented 4 years ago

I like to have my tests next to my components as well, but thats just me.

angelocordon commented 4 years ago

Thanks @tgrrr for writing up this issue and facilitating the discussion 🙏🏽🙏🏽 And for everybody else participating so far!

Re: Redux - I think we should use it over Context because Context, while so much simpler to use with hooks, forces your child components to rerender when the data inside a context changes. I don’t think there’s anything wrong with using Context but we should be more selective as to where we use it. I think it may be perfect for locally scoped components but terrible for root level state management.

We’ll probably need Redux sooner than we think, though no need to implement it yet until we write out that issue. That said, I’d prefer to keep our actions and reducers closer to the components they’re used in, instead of separately.

I’m not sure I’m a fan of a separate UI folder and don’t think this is any DRYer. This feels a bit redundant to me and confusing when we start to rename components as something else. It feels fine during the single example but if we’re going to start to customize a bunch of Material components, I feel that’s going to lead to some unscalable complexities. I would suggest we don’t fully customize Material components and work with what we have. For what it’s worth, we can most likely add data attributes to material components if that’s how we want to test things — but we really don’t need to test Material UI. We just need to test our own components.

We also set up StyledComponents, so any styles outside of Material will live inside the component file themselves. I’m not sure how we want to handle some of our global overrides though.

Lastly, I think Cypress adds itself in the src folder? If so, let’s not move that out and keep it in src. I feel like that’s a bit more appropriate.

Overall, this is pretty good! Looking forward to a PR!

lpatmo commented 4 years ago

lol, all the opinions!

@tgrrr I think you can continue with your original proposal. I think we're all in agreement on using hooks, but we can discuss these things later:

redux
test files in their separate folder vs next to each component _ a separate UI folder

Priority with the UI now is figuring out what exactly we want in the UI so I plan to focus on that, although I appreciate that you're making some organizational structural improvements to this codebase right now.

On Sat, Feb 8, 2020, 8:02 AM Angelo Cordon notifications@github.com wrote:

Thanks @tgrrr https://github.com/tgrrr for writing up this issue and facilitating the discussion 🙏🏽🙏🏽 And for everybody else participating so far!

Re: Redux - I think we should use it over Context because Context, while so much simpler to use with hooks, forces your child components to rerender when the data inside a context changes. I don’t think there’s anything wrong with using Context but we should be more selective as to where we use it. I think it may be perfect for locally scoped components but terrible for root level state management.

We’ll probably need Redux sooner than we think, though no need to implement it yet until we write out that issue. That said, I’d prefer to keep our actions and reducers closer to the components they’re used in, instead of separately.

I’m not sure I’m a fan of a separate UI folder and don’t think this is any DRYer. This feels a bit redundant to me and confusing when we start to rename components as something else. It feels fine during the single example but if we’re going to start to customize a bunch of Material components, I feel that’s going to lead to some unscalable complexities. I would suggest we don’t fully customize Material components and work with what we have. For what it’s worth, we can most likely add data attributes to material components if that’s how we want to test things — but we really don’t need to test Material UI. We just need to test our own components.

We also set up StyledComponents, so any styles outside of Material will live inside the component file themselves. I’m not sure how we want to handle some of our global overrides though.

Lastly, I think Cypress adds itself in the src folder? If so, let’s not move that out and keep it in src. I feel like that’s a bit more appropriate.

Overall, this is pretty good! Looking forward to a PR!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codebuddies/react-concept/issues/76?email_source=notifications&email_token=ABCNXO4D52GQOQSXMYZ6CJDRB3JSNA5CNFSM4KQHQOMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELFVBQY#issuecomment-583749827, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNXOZMCBYRB3RQ6BN7IN3RB3JSNANCNFSM4KQHQOMA .

tgrrr commented 4 years ago

Similar to @angelocordon, a big thanks to everyone who is contributing to this conversation! If you know me well, I don't use exclamation points lightly.

Re: Cypress: It automatically adds its own /Cypress folder outside of /src. It looks possible to customise the integrations folder in cypress config with integrationFolder see: https://docs.cypress.io/guides/references/configuration.html#Options. I haven't tried it as yet, but that would make it possible to set the integrations in the /src directory, beside each component.

What global overrides do we currently need to account for? It's definitely something we'll need to do, but I'm asking because of folder structure considerations.

I just remembered another pattern that I liked, and will add a /hooks folder, and a /services/api folder, because we'll definitely need those moving forward

tgrrr commented 4 years ago

Do we need a /helpers folder?

angelocordon commented 4 years ago

Cool, if Cypress by default adds it there, let’s keep it there. The less configuration we have to do in this step, the better (save you some headache).

I’d also suggest that we don’t preoptimize too much at this stage - let’s add folders when we have use for them 👍🏽

lpatmo commented 4 years ago

I think we should tackle this soon. I'm itching to add to the prototype a bit more, and would be good to have this structure in beforehand.

angelocordon commented 4 years ago

Agreed - maybe we can start small if this issue is too big. I think just having a separate /pages folder from /components would be a huge unblocker, tbh. As we're about to start handling routing and authentication, that would be a huge help. We can worry about handling all the other cases in separate issues later.

Do you think that would make this easier @tgrrr?

tgrrr commented 4 years ago

I think we should tackle this soon. I'm itching to add to the prototype a bit more, and would be good to have this structure in beforehand.

I'm currently working on this @lpatmo

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tgrrr commented 4 years ago

I did some work on this, but it became redundant with the changes that were made.

I still think this is important, and needs to be move out of Done @lpatmo @angelocordon

lpatmo commented 4 years ago

I agree! Was going to work on this after the React Query refactor

On Sun, Sep 20, 2020, 9:27 PM Phil notifications@github.com wrote:

I did some work on this, but it became redundant with the changes that were made.

I still think this is important, and needs to be move out of Done @lpatmo https://github.com/lpatmo @angelocordon https://github.com/angelocordon

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codebuddies/frontend/issues/76#issuecomment-695894386, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNXO3EG2SHPLTNCIM73ILSG3I2PANCNFSM4KQHQOMA .

angelocordon commented 4 years ago

Same. If you have something put together, feel free to put up a PR anyways @tgrrr

angelocordon commented 4 years ago

Let’s scope this down to pulling route components (pages) into a pages directory

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

codebuddies / frontend

Restructure folders #76