search

openedx / frontend-app-authoring

Front-end for the Open edX Studio experience, implemented in React and Paragon.
GNU Affero General Public License v3.0
13 stars 75 forks source link

[Editors] Redux/Folder organization #1239

Open connorhaugh opened 1 year ago

connorhaugh commented 1 year ago

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 commented 1 year ago

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

connorhaugh commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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