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

Updates electron and associated dependencies to the latest #2102

Merged
merged 18 commits into from
Jun 18, 2020

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented May 18, 2020

Fix

  • This updates all the Electron dependencies
  • Removes electron-spellchecker because Electron now has one built-in.

Should fix: #2031

Test

  • Run the checklist on the artifacts on this PR
  • Test spellcheck.
  • Does it turn off and on correctly?
  • Test all the menu items in electron.

@belcherj belcherj changed the title Update/electron Updates electron and associated dependencies to the latest May 19, 2020
@belcherj belcherj self-assigned this May 19, 2020
@nsakaimbo
Copy link

@belcherj This is a good start! Simplenote currently doesn't have spellchecking as a preference (or does it?).

In order to enable spellchecking, you'll need to toggle spellcheck in the window's web preferences to true. In wp-desktop, that value is loaded from the user's "spellcheck enabled" setting at app start. (Also, it's not overridable at runtime, so requires an application restart).

@belcherj belcherj requested a review from codebykat June 3, 2020 20:14
@belcherj belcherj marked this pull request as ready for review June 3, 2020 20:14
@belcherj
Copy link
Contributor Author

belcherj commented Jun 3, 2020

Screen Shot 2020-06-03 at 4 27 14 PM

Screen Shot 2020-06-03 at 4 30 01 PM

@belcherj
Copy link
Contributor Author

belcherj commented Jun 3, 2020

Screen Shot 2020-06-03 at 4 37 02 PM

@belcherj
Copy link
Contributor Author

belcherj commented Jun 4, 2020

Fixed the above issues

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.

When I try to logout, it hangs on "Logging out...", console shows this crash:

boot.ts?1649:24 Uncaught (in promise) ReferenceError: require is not defined
    at eval (boot.ts?1649:24)
    at new Promise (<anonymous>)
    at clearStorage (boot.ts?1649:13)
    at eval (boot.ts?1649:99)
    at eval (middleware.ts?2ac7:97)

Otherwise this tests okay and the code looks good. Small question about the icon change.

// Create the browser window.
const iconPath = path.join(
__dirname,
'../lib/icons/app-icon/icon_256x256.png'
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean this icon is now unused, or is it being loaded somewhere else? I would like to remove it if possible (see #2158)

@codebykat
Copy link
Member

Still getting a hang on logout, but without any console errors now :/

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.

Logging out works now! 🙆‍♀️

One tiny question about the --inspect line but merge at will.

@@ -52,7 +52,7 @@ PUBLISH ?= onTag
# Main targets
.PHONY: start
start:
@NODE_ENV=$(NODE_ENV) DEV_SERVER=$(DEV_SERVER) npx electron .
@NODE_ENV=$(NODE_ENV) DEV_SERVER=$(DEV_SERVER) npx electron . --inspect
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to leave this debugging in for start and not just dev? Not sure if I am reading it right.

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 left it there on purpose.

Copy link
Member

Choose a reason for hiding this comment

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

why?

also why did we remove rebuild-deps?

@belcherj belcherj merged commit d4b3c2c into develop Jun 18, 2020
@belcherj belcherj deleted the update/electron branch June 18, 2020 21:29
@codebykat codebykat added this to the 1.20 milestone Jun 29, 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.

Print only works once per session
4 participants