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

Add Checklist icon to note toolbar #2603

Merged
merged 2 commits into from
Jan 29, 2021
Merged

Conversation

sandymcfadden
Copy link
Contributor

@sandymcfadden sandymcfadden commented Jan 27, 2021

Fix

This addresses part of #2563 and adds a checklist icon to the left of the note toolbar. This allows clicking the icon to start a checklist instead of relying on keyboard shortcuts or electron menu items.

A big part of this is thanks to @codebykat for coming up with a way to communicate with the editor instance from outside its component.

Screen Shot 2021-01-27 at 1 14 00 PM
Screen Shot 2021-01-27 at 1 13 44 PM

Test

  1. In a note click into the note where you want to start a checklist
  2. Click the checklist icon in the top note toolbar.
  3. Notice a checklist is started.
  4. Highlight a few lines in a note.
  5. Click the checklist icon again.
  6. Notice all selected lines are turned in checklist items.

Release

  • Add checklist icon to note toolbar to allow easily starting checklists.

@sandymcfadden sandymcfadden added this to the 2.7.0 milestone Jan 27, 2021
@sandymcfadden sandymcfadden requested a review from a team January 27, 2021 17:15
@sandymcfadden sandymcfadden self-assigned this Jan 27, 2021
@codebykat codebykat self-requested a review January 29, 2021 09:55
Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

I picked @dmsnell's brain about this a bit, here is some copy-pasting:

Dennis:

one way to explore, not sure if it’d be nice, is to take that event dispatching code from the PR and put it into somewhere like how we have the search-field middleware, possibly rename that file and put them both there.
fire the redux action, middleware sees action, calls window.dispatchEvent yada yada
inside the editor we call the editor function directly. outside of the editor we fire the action and the middleware calls the window event which the editor traps
not the most important since we only have one place for inserting a task, though we could also make the keyboard hotkey call this, call it in tests, etc…

I noted that we would need to add a new action, because right now the editor dispatches INSERT_TASK after running the toggle checklist action (so using that same action would cause it to call itself)

Dennis:

we could change that so the editor calls the action and then all updates happen in response to the Redux action

(I don't love this idea because right now we're handling it with a Monaco command, which doesn't have to bounce out to the rest of the app at all)

Dennis:

or just leave it for now in the PR and add mega comments

(mega comment added ✅ )

As far as next steps on this PR go, IMO it can be merged as-is. We are only calling this from a single place outside the editor, and the Electron menus already use their own communication channel (all the appCommand and editorCommand stuff).

If we want to add another action to the middleware, I would advocate for renaming the existing action to clarify that it is triggered by the toggle checklist action, rather than triggering it.

@sandymcfadden
Copy link
Contributor Author

Thank you both very much for your thoughts and help on this!

@sandymcfadden sandymcfadden merged commit 1d5117c into develop Jan 29, 2021
@sandymcfadden sandymcfadden deleted the add/checklist-icon branch January 29, 2021 12:05
sandymcfadden added a commit that referenced this pull request Jan 29, 2021
@pachlava
Copy link
Contributor

[testing] fix/feature verified on Dell Latitude 7400 | Win 10 | Simplenote 2.7.0-beta1

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