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 loading actions #2996

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Conversation

adityagarg06
Copy link
Contributor

Progress on #2042
Changes:

  • Used the createSlice to create the loading reducer.
  • Deleted the loader action file as createSlice automatically creates actions for the reducer.
  • Removed the loader action constants.
  • Changed the import path for loader actions in assets.js, collections.js, and projects.js.
  • Connected the Legal.jsx to redux to use the loading state from the redux store instead of handling it locally.
  • Improved the loading state in AddToCollectionList and AddToCollectionSketchList.
  • Replaced the action types related to loading in the projects.unit.test.js file.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@lindapaiste
Copy link
Collaborator

lindapaiste commented Feb 6, 2024

This is good stuff but I think that I need to explain my // TODO: improve loading state comment. We have a single state.loading for the entire app. It doesn't tell us what is loading. In the long term we would want to know that this specific thing is loading, possibly with RTK query? As a short-term bandaid fix I put in the hasLoadedData state and the more complicated check of const showLoader = loading && !hasLoadedData;. This prevents a situation where the loader shows up due to some unrelated API call elsewhere on the page.

There was a situation that was previously occurring where the sketch list would go into a loading state when you clicked the "add to collection" dropdown, as the "Add to collection" modal has its own API call which sets the global state.loading to true. TBH the two components where you changed it are probably fine? Since those are the secondary API calls. But I was putting that check in on all the components that I refactored to be on the safe side.

@@ -33,9 +33,9 @@ describe('projects action creator tests', () => {
store = mockStore(initialTestState);

const expectedActions = [
{ type: ActionTypes.START_LOADING },
{ type: 'loading/startLoader' },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either of these should work:

{ type: startLoader.type },
startLoader(),

startLoader() is an action creator, so it's a function that returns the action. If you just call that function without dispatching you will get the value { type: 'loading/startLoader' }.

Comment on lines 39 to 40
dispatch(startLoader());
dispatch(getCollections(username)).then(() => dispatch(stopLoader()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the source of the getCollections thunk. I think that it dispatches startLoader and stopLoader on its own already?

Copy link
Collaborator

@lindapaiste lindapaiste Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is subjective. Personally I would use the local component state. The policy loading state is not used in any other component so I don't think that it needs to be in a global Redux state.

…ist, AddToCollectionSketchList and Legal components
@adityagarg06
Copy link
Contributor Author

adityagarg06 commented Feb 8, 2024

I have reverted the changes made in the AddToCollectionList, AddToCollectionSketchList, and Legal. I will create a new PR to implement them using the RTK query. Also made the change that you requested in the project.test file. Thanks for the feedback.

@lindapaiste lindapaiste merged commit a9e518c into processing:develop Feb 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants