processing / p5.js-web-editor

The p5.js Editor is a website for creating p5.js sketches, with a focus on making coding accessible and inclusive for artists, designers, educators, beginners, and anyone else! You can create, share, or remix p5.js sketches without needing to download or configure anything.
https://editor.p5js.org
GNU Lesser General Public License v2.1
1.3k stars 1.26k forks source link

refactor search && sorting reducers && actions using redux toolkit #3124

Open PiyushChandra17 opened 1 month ago

PiyushChandra17 commented 1 month ago

Associated issue: #2042

Changes:

I have verified that this pull request:

PiyushChandra17 commented 2 weeks ago

@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 commented 2 weeks ago

@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. https://github.com/processing/p5.js-web-editor/blob/7e0827805db71868151bdc7783385992446cc859/client/modules/IDE/actions/sorting.js#L8-L16

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.

PiyushChandra17 commented 1 week ago

@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