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

State Refactor: Move note info into Redux #1896

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

codebykat
Copy link
Member

@codebykat codebykat commented Feb 12, 2020

Fix

This moves the showNoteInfo boolean and associated toggles into Redux state.

Test

  1. Click the note details pane and make sure note details is shown
  2. Click the X or outside the pane and make sure it toggles closed

Review

Code review: Please check to make sure I did this right and didn't miss anything! I used #1881 as an example.

I moved showNoteInfo from appState.showNoteInfo to state.ui.showNoteInfo, added new actions and a new reducer, and removed the legacy action.

In the reducer I am definitely eyeing VisiblePanes as potentially a more logical place to put this (along with the Navigation, see below) but this PR seemed complicated enough already so I decided to stick with a statewide boolean for now.

Another thing of note is that the former incarnation of this action also sets showNavigation and editingTags to false when the notes panel is shown. I was unable to figure out what edge cases that was meant to handle, so I'm not sure if it is still necessary, and it seems it will be somewhat complicated without growing the PR to also move those two state variables into Redux. (See my line comments on the diff below.)

@codebykat codebykat requested a review from a team February 12, 2020 05:25
@codebykat codebykat self-assigned this Feb 12, 2020
lib/flux/app-state.ts Outdated Show resolved Hide resolved
// showNavigation: { $set: false },
// editingTags: { $set: false },
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, I believe we don't need to handle these cases explicitly, but I hope someone can verify that

Copy link
Member

Choose a reason for hiding this comment

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

we can verify it!

Copy link
Member

Choose a reason for hiding this comment

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

in testing I can see that it's more of a choice to change the behavior or keep it. if we take this out then the note info stays open when we flip the navigation panel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't been able to make that happen, what are the steps? If I open note info, there's no way to open the navigation panel (any click is an "outside click" that closes the note info).

Copy link
Member Author

Choose a reason for hiding this comment

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

From Slack: "Use the keyboard shortcut Cmd + t and flip the tag drawer/nav panel open and closed"

I tried this and it does seem to close the info panel, so I'm gonna say.. it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

sorry. I should have clarified that - obviously this code closes the note info panel when toggling the navigation panel. however, when I removed the case 'App.toggleNavigation' as it seemed like you were suggesting would be possible, then the note info would stay open on navigation.

That is, this is an answer to the question "can we skip handling these cases explicitly" and that answer is "if we want and decide to."

lib/state/ui/reducer.ts Outdated Show resolved Hide resolved
lib/note-toolbar/index.tsx Outdated Show resolved Hide resolved
lib/note-toolbar/index.tsx Outdated Show resolved Hide resolved
lib/note-info/index.tsx Outdated Show resolved Hide resolved
lib/flux/app-state.ts Outdated Show resolved Hide resolved
@dmsnell
Copy link
Member

dmsnell commented Feb 12, 2020

Lookin' good @codebykat - I hope you found the examples and documentation helpful. Left a few notes which I think might make your job easier. If not, don't hesitate to reach out and challenge or ask.

@belcherj
Copy link
Contributor

Looks great, I didn't see anything outside of what Dennis came up with!

@belcherj
Copy link
Contributor

@codebykat
Copy link
Member Author

Cool, that all makes sense! Fixed some nits and up for review again. Thanks @dmsnell :)

One thing is I couldn't figure out how to remove the dispatch argument from MapDispatchToProps in note-info (here):

Screen Shot 2020-02-12 at 10 07 21 AM

It works in note-toolbar (here) and I'm not sure what the difference is:

Screen Shot 2020-02-12 at 10 05 54 AM

There is some other stuff that needs to be moved to Redux in note-detail, and I don't think I understand the onOutsideClick situation, so maybe that's why?

lib/note-info/index.tsx Outdated Show resolved Hide resolved
lib/note-toolbar/index.tsx Outdated Show resolved Hide resolved
@codebykat codebykat force-pushed the refactor/move-note-info-to-redux branch from 735a7e0 to 710d0e5 Compare February 13, 2020 19:29
@codebykat
Copy link
Member Author

Rebased and tested both this and the edit mode toggle (which was entangled).

@codebykat codebykat force-pushed the refactor/move-note-info-to-redux branch from 299cce8 to 91e6aa0 Compare February 13, 2020 21:22
@codebykat codebykat requested review from dmsnell and a team February 14, 2020 02:46
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Seems like a good merge and I don't think we need to worry about asking whether we should handle the App.toggleNavigation action or not. As-is the code matches the existing behavior so that's an easier call to make when refactoring than whether we should change behavior.

@codebykat codebykat merged commit 85bae61 into develop Feb 14, 2020
@codebykat codebykat deleted the refactor/move-note-info-to-redux branch February 14, 2020 21:27
@dmsnell dmsnell mentioned this pull request Feb 19, 2020
41 tasks
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.

4 participants