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

Refactors closeNote to Redux #1895

Merged
merged 8 commits into from
Feb 17, 2020
Merged

Refactors closeNote to Redux #1895

merged 8 commits into from
Feb 17, 2020

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Feb 11, 2020

Fix

Refactors closeNote moving the function from the flux actions to Redux. In doing this it required the removal of the note list open/closed that was being stored in the component state of app.tsx.

Test

  1. Have SImplenote open with a small width so only the note list appears
  2. Click a note
  3. Click the back button at the top
  4. Click note again
  5. Trash it, does note list appear?
  6. Open Trashed note list
  7. Click into note
  8. Click back button at top
  9. Permanently trash note, should go back to trashed note list
  10. Empty trash, should see blank note editor
  11. Go back to all notes
  12. Create new note
  13. Play with search

@belcherj belcherj self-assigned this Feb 12, 2020
@belcherj belcherj requested a review from a team February 12, 2020 17:45
@belcherj belcherj marked this pull request as ready for review February 12, 2020 17:45
@belcherj belcherj force-pushed the refactor/closeNoteAction branch 2 times, most recently from 088a8f7 to 6f07ebb Compare February 13, 2020 20:41
lib/state/ui/reducer.ts Outdated Show resolved Hide resolved
@belcherj belcherj force-pushed the refactor/closeNoteAction branch from 6f07ebb to d86ab66 Compare February 17, 2020 15:09
@belcherj belcherj merged commit 586dadc into develop Feb 17, 2020
@belcherj belcherj deleted the refactor/closeNoteAction branch February 17, 2020 16:26
closeNote,
toggleEditMode,
toggleNoteInfo,
};
Copy link
Member

Choose a reason for hiding this comment

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

😎

(settings.focusModeEnabled || !isSmallScreen)
),
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Earmarking to test…


default:
return state;
}
Copy link
Member

Choose a reason for hiding this comment

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

For actions like deleteNoteForever and restoreNote and the like is this now handled automatically by some other code, that the note closes?

Copy link
Member

Choose a reason for hiding this comment

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

Noted in testing: the note closes automatically due to the filtering middleware which implicitly unselects it as it no longer exists.

}

return state;
};
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for removing this. Probably would have been nice to do it in a separate PR with its own traceable history, or at least a mention in the git commit, since now we don't have any explanation for why this was removed in the code or git history - it just disappears.

if (note) {
note.data.deleted = true;
noteBucket.update(note.id, note.data);

dispatch(this.action('closeNote', { previousIndex }));
Copy link
Member

Choose a reason for hiding this comment

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

Are we removing the functionality that the previousIndex provided?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was broken in #1851 so it won't appear in testing of this PR

dmsnell added a commit that referenced this pull request Feb 21, 2020
In #1895 we added an import to `./state` that should have been
`../state`. This didn't break the app because the import only
affected the types.

In this patch we're fixing the import path.
belcherj pushed a commit that referenced this pull request Feb 21, 2020
In #1895 we added an import to `./state` that should have been
`../state`. This didn't break the app because the import only
affected the types.

In this patch we're fixing the import path.
@codebykat codebykat added this to the state refactor milestone Oct 21, 2020
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.

3 participants