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 (project): component state cleanup #623

Closed
7 of 10 tasks
ciyer opened this issue Sep 18, 2019 · 3 comments
Closed
7 of 10 tasks

Refactor (project): component state cleanup #623

ciyer opened this issue Sep 18, 2019 · 3 comments

Comments

@ciyer
Copy link
Contributor

ciyer commented Sep 18, 2019

We have too many redux stores; we should have just one redux store for the entire application. If a component needs local state, it should use react hooks.

While doing this, we should also reduce to a minimum the information passed to sub-components of the project page. Currently, they get the full project state, but they should only receive what they need.

Here are the locations where stores are created:

createStore

StateKind.REDUX

mix

StateKind.REACT

  • QuickNav.container.js: QuickNavContainerWithRouter --> this doesn't necessarily need to be moved since data are stored in a local react state. We may still want to move it to the global redux store, but I can't see many synergies now (that may change with a search bar using a KG query)
@ciyer ciyer added the Epic label Sep 18, 2019
@lorenzo-cavazzi
Copy link
Member

A couple of ideas to keep in mind while moving to a single redux store:

  • gradually move nested data to the first level if it's feasible. E.G. from this:

     projects: {
       project: {
     	commits: {},
     	branches: {}
       }
     }
    

    to this:

     {
     	projects: {},
     	project: { id:123, ... }
     	branches: { projectId:123, ... }
     	commits: { projectId:123, branch:"master", ... }
     }
    

    The main benefit is that it's easier for components to mapStateToProps to a specific topic. It's also easier to handle stale updates after involking an API

  • stop using setUpdating/ "is_updating" as defined in Models.js because it overwrites the object values and it's generally not desirable.
    Instead, try to use an extra property status: { fetched: <timestemp>, fetching: <bool>} as suggested in Server Notifications Middleware #225

  • As a general rule, we can try to keep in the redux store all the data coming from the backend and map them to props directly where needed instead of passing props down (this avoids undesired refreshes). All the other data (usually temporary data or UI-related data) should be stored in react state/props

@lorenzo-cavazzi
Copy link
Member

ProjectList and QuickNav may be the next step since the content seems to be about projects and it may share functions with the already existing ProjectsCoordinator

@ciyer ciyer changed the title State Management Refactor (project): component state cleanup Dec 4, 2020
@lorenzo-cavazzi
Copy link
Member

The cleanup here was mostly done. We need additional refractory but we will open dedicated issues for that

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

2 participants