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

Move editor mode to redux #1866

Merged
merged 1 commit into from Feb 13, 2020
Merged

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Jan 27, 2020

Fix

This PR moves the editor mode state from flux to redux.

Test

  1. Open app
  2. Click into note with markdown
  3. Does the note display in edit mode?
  4. Click preview button
  5. Does note display in preview mode
  6. Click between notes
  7. Does preview mode remain enabled
  8. Turn off preview mode
  9. Does preview mode turn off
  10. Turn preview back on
  11. Turn off markdown in the info panel for a note
  12. Does preview mode turn off?

Review

Only one developer is required to review these changes, but anyone can perform the review.

lib/flux/app-state.ts Outdated Show resolved Hide resolved
@belcherj belcherj changed the title WIP: Move editor mode to redux Move editor mode to redux Jan 27, 2020
@belcherj belcherj marked this pull request as ready for review January 27, 2020 23:42
@belcherj belcherj requested a review from a team January 27, 2020 23:42
@belcherj belcherj self-assigned this Jan 27, 2020
lib/state/ui/reducer.ts Outdated Show resolved Hide resolved
@belcherj belcherj added the state label Jan 29, 2020
@belcherj belcherj force-pushed the refactor/move-editor-mode-redux branch from 1ea0f5c to 8298de2 Compare January 29, 2020 15:08
@belcherj
Copy link
Contributor Author

@dmsnell @codebykat ready for more review

@belcherj
Copy link
Contributor Author

@dmsnell @codebykat please rereview

lib/types.ts Show resolved Hide resolved
lib/flux/app-state.ts Outdated Show resolved Hide resolved
@@ -129,7 +129,7 @@ const mapDispatchToProps = dispatch => ({
deleteNoteForever: args => dispatch(deleteNoteForever(args)),
noteRevisions: args => dispatch(noteRevisions(args)),
restoreNote: args => dispatch(restoreNote(args)),
setEditorMode: args => dispatch(setEditorMode(args)),
setEditorMode: mode => dispatch(setEditorMode(mode)),
Copy link
Contributor

@dmsnell dmsnell Jan 29, 2020

Choose a reason for hiding this comment

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

No need to do all the work in this PR, but these actions somewhat subvert our normal Redux pattern. All we're doing is passing them to the child component, so how about moving them properly while we're fixing the other aspect of this one Redux state type.

That is, can we eliminate this action from the <NoteToolBar /> container and instead put it in <NoteToolbar /> where it's used?

In that file it looks like we're using state-based logic as well when all we care about doing is toggling. If we wanted, we could further remove the duplicated logic and create a second new action-type and let the reducer centralize and isolate that logic…

// action-types
export type SetEditorMode = Action<'SET_EDITOR_MODE', { mode: T.EditorMode }>;
export type ToggleEditorMode = Action<'TOGGLE_EDITOR_MODE'>;

// reducer
const editorMode: A.Reducer<T.EditorMode> = (state = 'edit', action) => {
   switch (action.type) {
     case 'TOGGLE_EDITOR_MODE':
       return state === 'edit' ? 'markdown' : 'edit';
     case 'SET_EDITOR_MODE':
     case 'App.setEditorMode':
       return action.mode;
     case 'App.newNote':
       return 'edit';
     default:
       return state;
   }
 };

Just mentioning the toggler because it stood out to me in <NoteToolbar /> that we're doing extra logic because we didn't have a semantic way of indicating what we wanted to do and because it's a small change while updating the edit mode logic. It's not essential but might be worth it and a good example of where Redux lets us think more about semantic app interactions than merely being a bag of global setters.

lib/note-editor/index.tsx Outdated Show resolved Hide resolved
@dmsnell
Copy link
Contributor

dmsnell commented Jan 29, 2020

It looks like if we handle App.newNote in the editorMode reducer we can eliminate the old method altogether in app-state. For some reason I remember having an issue with that, maybe because newNote is a creator? If that's the case we might find it more appealing in the short-term to stub out a NEW_NOTE or CREATE_NOTE (I like "create" since it seems less ambiguous to receiving a new note from the server) action and trap on that

// action-types
export type CreateNote = Action<'CREATE_NOTE'>;

// editorMode reducer
case 'CREATE_NOTE':
  return 'edit';

Later on that action type could be filled-in with additional necessary properties when we remove it from app-state

@belcherj belcherj force-pushed the refactor/move-editor-mode-redux branch from 5820731 to 6866e85 Compare February 5, 2020 15:37
import * as A from '../action-types';
import * as T from '../../types';

const defaultVisiblePanes = ['editor', 'noteList'];
const emptyList: unknown[] = [];

const editMode: A.Reducer<boolean> = (state = true, action) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

changing from named states to boolean seems like a semantic regression.

does true or false mean "preview"?

I can follow the idea if we're saying "edit mode" is the mode, though particularly since we talk about edit vs. preview when discussing this I personally find 'edit' and 'preview' clearer in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reasoned about this for a while. I'd love to chat about it but I came to the opposite conclusion. We are in either editor more or preview. If edit mode is true then we are in preview. Just as is the tag panel open or closed, or are we in focus mode or is the note list open.

Remove print state and call print directly (#1872)

This PR calls the print function directly from app.tsx rather than setting up state. By not having to set editor/preview mode we decrease the complexity of this functionality while giving the user the option to print the editor view OR the markdown styled mode.

Before this change, the print function was intended to only print the formatted markdown mode but the functionality is broken. This PR removes the broken code.
@belcherj belcherj force-pushed the refactor/move-editor-mode-redux branch from e4c7a7c to 380825c Compare February 11, 2020 15:27
@belcherj belcherj merged commit 07e16ee into develop Feb 13, 2020
@belcherj belcherj deleted the refactor/move-editor-mode-redux branch February 13, 2020 19:05
@dmsnell dmsnell mentioned this pull request Feb 19, 2020
41 tasks
@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.

None yet

3 participants