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

[Editors] Redux/Folder organization #1239

Open
connorhaugh opened this issue Feb 9, 2023 · 5 comments
Open

[Editors] Redux/Folder organization #1239

connorhaugh opened this issue Feb 9, 2023 · 5 comments

Comments

@connorhaugh
Copy link
Contributor

connorhaugh commented Feb 9, 2023

Folder structure proposal.
We Currently have this folder structure:

Editors
|-- Containers
|   |-- EditorContainer
|   |   `-- React Components
|   |-- ProblemEditor
|   |   `-- React Components
|   |-- TextEditor
|   |   `-- React Components
|   `-- VideoEditor
|       `-- React Components
|-- EditorPage
|   `-- React Components
`-- Data
    |-- Redux
    |   |-- App
    |   |   |-- Selectors, 
    |   |   |-- Reducers, 
    |   |   `-- etc
    |   |-- Problem
    |   |   |-- Selectors, 
    |   |   |-- Reducers, 
    |   |   `-- etc
    |   |-- Video
    |   |   |-- Selectors, 
    |   |   |-- Reducers, 
    |   |   `-- etc
    |   |-- Requests
    |   |   |-- Selectors, 
    |   |   |-- Reducers, 
    |   |   `-- etc
    |   `-- ThunkActions
    |       |-- App
    |       |-- Problem
    |       `-- Video
    `-- Services
        |-- Api
        `-- Urls

This has two problems:

  1. Where data is dealt with is a hodgepodge of overlapping code
  2. It does not clearly separate the framework and editor layers of the project.

Therefore, we should have this folder structure:

SRC
|-- Editors
|   |-- ProblemEditor
|   |   |-- Data
|   |   |   |-- Constants
|   |   |   `-- Redux
|   |   |       |-- Selectors
|   |   |       |-- Reducers
|   |   |       |-- Thunkactions
|   |   |       `-- index
|   |   |-- components
|   |   `-- index.jsx
|   |-- TextEditor
|   |   |-- Data
|   |   |   |-- Constants
|   |   |   `-- Redux
|   |   |       |-- Selectors
|   |   |       |-- Reducers
|   |   |       |-- Thunkactions
|   |   |       `-- index
|   |   |-- components
|   |   `-- index.jsx
|   `-- VideoEditor
|       |-- Data
|       |   |-- Constants
|       |   `-- Redux
|       |       |-- Selectors
|       |       |-- Reducers
|       |       `-- Thunkactions
|       |-- components
|       `-- index.jsx
`-- Framework
    |-- EditorContainer
    |-- EditorPage
    `-- Data
        |-- Constants
        |-- Redux
        |   |-- Selectors
        |   |-- Reducers
        |   |-- Thunkactions
        |   `-- index
        `-- Serivces
            |-- api
            `-- urls
@jristau1984
Copy link
Contributor

@connorhaugh great proposal! Some questions I have...

  • In your thought experiment in coming up with this proposal, did you have other options that you rejected that you could list here?
  • Can you foresee any downsides or restrictions that we may want to plan for/accept as new truths in your proposed structure?

@connorhaugh
Copy link
Contributor Author

In your thought experiment in coming up with this proposal, did you have other options that you rejected that you could list here?

There were three choices here:

  1. move each editor's redux layer into the editor's folder
  2. Create a framework folder, put all the framework things in there
  3. put the ThunkActions for each layer next to the respective layer.

Can you foresee any downsides or restrictions that we may want to plan for/accept as new truths in your proposed structure?

  1. This would require some code changes
  2. We need to think about where the sharedComponents will go

@jesperhodge
Copy link
Member

jesperhodge commented Feb 16, 2023

@connorhaugh I agree with Jeremy, this is an excellent proposal. It solves the problem of separating the framework concerns from the internal application concerns, and it cleans up the confusing redux structure by putting the data folder and slices directly with the editors they supplement.

I see that this extends to a revamping of the whole folder structure, and not just the redux part of the application. Which I think is a good thing. I think if you come from the pure React part of how this project is structured, we anyway need some changes, and I was going to suggest folder structure changes for that sometime in the future. I think it would be good to find our common ground here, and can go through everything just one time to create a very good folder structure that is sustainable in the long term.

Right now you have a components folder inside of each of the editors. I was wondering how you wanted to structure this. I suggest to put all the react components that are inside an editor inside there, right next to each other. No more nesting.

Right now an example looks like this:

SRC
|-- Editors
| |-- ProblemEditor
| | |-- components
|. |. |. |-- EditProblemView
|. |. |. |. |-- AnswerWidget
|. |. |. |. |. |-- components
|. |. |. |. |. |. |-- Checker
|. |. |. |. |. |. |-- Feedback
|. |. |. |. |. |-- AnswerOption
|. |. |. |. |. |-- AnswersContainer

and this can go down many more levels of nesting. Much better would be:

SRC
|-- Editors
| |-- ProblemEditor
| | |-- components [only components that are so related to ProblemEditor that it is helpful to keep them here]
|. |. |. |-- EditProblemView [this makes sense here, because it is not something we would want to reuse as is]
.... and so on
|-- Framework
|-- components [most components should live here. This is the default, because components should be reusable]
|. |-- AnswerOption
|. |-- AnswersContainer
|. |-- AnswerWidget
|. |-- Checker
|. |-- Feedback

The naming is also improvable, for example, SettingsOption is functionality wise actually just a collapsible card (it has absolutely no logic that is related to answers), so we should call it CollapsibleCard and then we can reuse it in many other places.

My concern with our current folder structure is that we have a deeply nested (and chaotic) structure where components are in other components' folders are in other components' folders and so on. This may seem intuitive, but in reality it is quite bad. It completely prevents components from being reused, and it also means when we build something we have a big discovery problem with existing components. I have been in more than one project that started with such a nested folder structure and we successfully moved to a much better non-nested folder structure because the former didn't work well enough.

If you do a search for community discussions about React folder structure on the internet, what will be the common consensus and confirms my point is that 1. nested components are bad for multiple reasons 2. the most popular folder structure are something extremely flat like a pages and a components folder with ComponentA, ComponentB, ComponentC living next to each other and 3. because applications can grow extremely complex, flat hierarchies have their limits, so commonly grouping components by features but keeping them as flat as possible is a very good structure. features would mean I think something more like functionality instead of parent elements as we have them now.

@jesperhodge
Copy link
Member

Another point to consider is how other teams facing the same questions organized their codebase. This proposed ADR which is in progress defines a set of best practices for folder structure. When discussed in the fedx frontend group, it received very favorable feedback, and it is also very much in line with React community recommendations (https://profy.dev/article/react-folder-structure).

@jesperhodge
Copy link
Member

jesperhodge commented Apr 5, 2023

This is the tentative folder structure we are proposing after discussion. We are also proposing adopting the ADR on feature-based organization that our enterprise colleagues follow.

Details like exact folder naming are left a bit open for now, as it is tentative.

The next step is for me to put up this ADR and for the whole team to review it. Obviously additional feedback from outside our team is welcome and will be considered.

SRC
|-- features
|   |-- problem-editor
|   |   |-- Data
|   |   |   |-- Constants
|   |   |   `-- Redux
|   |   |       |-- Selectors
|   |   |       |-- Reducers
|   |   |       |-- Thunkactions
|   |   |       `-- index
|   |   |-- sub-feature-A
|   |   |-- sub-feature-B
|   |   `-- ProblemEditor.jsx
|   |   `-- index.jsx
|   |-- text-editor
|   |   |-- Data
|   |   |   |-- Constants
|   |   |   `-- Redux
|   |   |       |-- Selectors
|   |   |       |-- Reducers
|   |   |       |-- Thunkactions
|   |   |       `-- index
|   |   |-- sub-feature-A
|   |   |-- sub-feature-B
|   |   `-- TextEditor.jsx
|   |   `-- index.jsx
|   `-- video-editor
|       |-- Data
|       |   |-- Constants
|       |   `-- Redux
|       |       |-- Selectors
|       |       |-- Reducers
|       |       `-- Thunkactions
|   |   |-- sub-feature-A
|   |   |-- sub-feature-B
|   |   `-- VideoEditor.jsx
|       `-- index.jsx
|   `-- editor-unrelated-feature-A
|   `-- editor-unrelated-feature-B
`-- framework
    |-- EditorContainer
    |-- EditorPage
    `-- Data
        |-- Constants
        |-- Redux
        |   |-- Selectors
        |   |-- Reducers
        |   |-- Thunkactions
        |   `-- index
        `-- Serivces
            |-- api
            `-- urls

@bradenmacdonald bradenmacdonald changed the title Redux/Folder organization [Editors] Redux/Folder organization Aug 29, 2024
@bradenmacdonald bradenmacdonald transferred this issue from openedx/frontend-lib-content-components Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants