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

fix : Folders auto-expand on save #2543

Conversation

7070Shreyash
Copy link
Contributor

Fixes #2530
Changes:
This PR fixes the folder auto-expand bug by appropriately modifying the files state in redux , adding necessary variables to capture the previous state before saving .
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
Screencast.from.30-10-23.02.08.20.AM.IST.webm

@welcome
Copy link

welcome bot commented Oct 29, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already.

Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

There are definitely some issues here with regards to mutating objects. I also suspect that you are changing code in more places than what's necessary. Possibly all that you need are the 6 lines in the case ActionTypes.SET_PROJECT reducer (with some tweaks).

I have other thoughts that I will put in a comment on the issue.

action.files = action.files.map((file) => {
const corrospondingObj = state.find((obj) => obj.id === file.id);
if (corrospondingObj && corrospondingObj.fileType === 'folder') {
file.isFolderClosed = corrospondingObj.isFolderClosed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can avoid mutating the action like this:

case ActionTypes.SET_PROJECT:
    const files = action.files.map((file) => {
        const corrospondingObj = state.find((obj) => obj.id === file.id);
        if (corrospondingObj && corrospondingObj.fileType === 'folder') {
            const isFolderClosed = corrospondingObj.isFolderClosed || false;
            return { ...file, isFolderClosed };
        }
        return file;
    });
    return setFilePaths(files);

FYI there are some mutations in the existing code in this reducer😢. But let's not add any new ones.

I have to look at the code to remind myself what the difference is between NEW_PROJECT and SET_PROJECT. If we need to use the same code in both places then we can move it to a helper function. But I suspect that it's not needed for the NEW_PROJECT case because if it's a new project then the corrospondingObj would always be undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect that it's not needed for the NEW_PROJECT case because if it's a new project then the corrospondingObj would always be undefined.

I was wrong. It is needed in both places. But it seems that NEW_PROJECT and SET_PROJECT are identical so now I'm wondering why we have two actions that do the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @lindapaiste , Thank you for your invaluable and clear response . I have made the suitable changes.

@raclim raclim added this to the PATCH Release for 2.9.3 milestone Nov 8, 2023
@raclim raclim merged commit fb30a55 into processing:develop Nov 17, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Folders auto-expand on save
4 participants