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

Remove print state and call print directly #1872

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Jan 29, 2020

Fix

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.

Test

  1. Open electron
  2. From file menu click print
  3. Observe it prints the note
  4. Turn on markdown mode
  5. Print
  6. Observe that it prints formatted markdown

Review

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

@dmsnell
Copy link
Contributor

dmsnell commented Jan 29, 2020

Actually we need not overcomplicate this: in testing in develop it only prints the current view regardless of the editor mode, which means that I think we can skip all the waiting routines and just use the most basic form:

if ('printNote' === command.action) {
  return window.print();
}

@belcherj belcherj changed the title Try removing print state Remove print state and call print directly Jan 30, 2020
@belcherj belcherj requested a review from a team January 30, 2020 15:13
@belcherj belcherj self-assigned this Jan 30, 2020
@belcherj belcherj marked this pull request as ready for review January 30, 2020 15:17
@dmsnell
Copy link
Contributor

dmsnell commented Jan 30, 2020

Before this change, the print function would only print the formatted markdown mode.

In my testing the behavior is the same before and after. It's possible that the same timing issues we ran into in this PR were already present in develop though. To the best of a quick search I found that people were resorting to strange means in order to figure out when React had finished updating the DOM.

@belcherj
Copy link
Contributor Author

I think removing the functionality and giving the user more choice is better all around!

@belcherj
Copy link
Contributor Author

The code that is being removed by this PR tried to switch the mode to markdown before printing. That code is broken. This PR removes it.

@dmsnell
Copy link
Contributor

dmsnell commented Jan 30, 2020

Formatted-only print removed in #1013
Printing introduced in 0eb023f

Copy link
Contributor

@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.

Sweet - less code and no loss and one race condition eliminated!

@belcherj belcherj merged commit 5820731 into refactor/move-editor-mode-redux Jan 30, 2020
@belcherj belcherj deleted the try/remove-print-state branch January 30, 2020 16:38
belcherj added a commit that referenced this pull request Feb 5, 2020
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 added a commit that referenced this pull request Feb 11, 2020
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 added a commit that referenced this pull request Feb 13, 2020
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.
@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.

None yet

2 participants