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 search && sorting reducers && actions using redux toolkit #3124

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

PiyushChandra17
Copy link
Contributor

Associated issue: #2042

Changes:

  • First things first removed all the associated constants
  • Used createSlice to rewrite the sorting and search reducers
  • Exported the automatically generated action creator functions from actions/sorting.js
  • Deleted the existing action creator function

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

@PiyushChandra17
Copy link
Contributor Author

@raclim @lindapaiste I have fixed few errors in this PR, but i don't know why the heck this sorting.field is returning undefined. Probably i need to look on this one and give you guys update as early as possible. I need to dig into the code and see where the fault lies ...

@lindapaiste
Copy link
Collaborator

@raclim @lindapaiste I have fixed few errors in this PR, but i don't know why the heck this sorting.field is returning undefined. Probably i need to look on this one and give you guys update as early as possible. I need to dig into the code and see where the fault lies ...

I do! The previous setSorting action took two arguments setSorting(field, direction). In the action creator function we restructured that into payload with two properties.

export function setSorting(field, direction) {
return {
type: ActionTypes.SET_SORTING,
payload: {
field,
direction
}
};
}

The new setSorting action creator that is auto-generated from createSlice takes one argument and that argument becomes the action.payload.

You're going to have the same problem with the setSearchTerm action which also currently takes two arguments setSearchTerm(scope, searchTerm).

There's two ways to fix it:

  1. Find every place in the code where we call the setSearchTerm action and replace the action calls setSearchTerm(scope, term) with setSearchTerm({ scope, query: term }).
  2. Make it so that the setSearchTerm action still accepts two arguments using the prepare feature of createSlice: https://redux-toolkit.js.org/api/createSlice#customizing-generated-action-creators
reducers: {
  setSearchTerm: {
    reducer: (state, action) => {
      const { scope, query } = action.payload;
      state[`${scope}SearchTerm`] = query;
    },
    prepare: (scope, query) => ({ payload: { scope, query } })
  }
}

I'm using setSearchTerm as the example there instead of setSorting because when I looked to see where we actually call setSorting...currently the only place is inside the resetSorting action. Personally I would move that action into the reducer and have resetSorting: () => { return initialState; }. So currently we don't even really need setSorting at all, though I can see how we might in the future, depending on what happens with PR #2376.

Comment on lines 14 to 17
return {
...state,
[`${scope}SearchTerm`]: query
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to do ...state stuff when using Redux Toolkit. The reducer uses a proxy of the actual state so it is okay to mutate it. You can return a new state to replace the entire state, but typically you just modify the state and don't return anything. It can be written as:

setSearchTerm: (state, action) => {
  const { scope, query } = action.payload;
  state[`${scope}SearchTerm`] = query;
}

@PiyushChandra17
Copy link
Contributor Author

@lindapaiste @raclim I think the overall functionality works now, we are able to sort and search but one unit-test is getting failed, probably need to look at this one and update as early as possible

@emad-ansari
Copy link

Hey, @lindapaiste , Is the issue open , can I work on this issue?

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.

4 participants