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 spellchecker (electron-spellchecker) #821

Merged
merged 83 commits into from
Sep 13, 2018
Merged

Conversation

adlk
Copy link
Contributor

@adlk adlk commented Sep 3, 2018

This is an alternative approach to #753 that is using electron-spellchecker.

I'm extending some electron-spellchecker classes to provide more flexibility and to mimic the setup of #753

This PR introduces the new make targets:

  • make dev: starting the webpack dev server
  • make start: start electron and check if a native dependency rebuild is necessary
  • make rebuild-deps: rebuilds native dependencies. This is a dependency of make start

@adlk
Copy link
Contributor Author

adlk commented Sep 6, 2018

@mirka

Can we keep all the file/folder names spinal-cased to match the rest of the codebase?

yup, I will update the file/folder names.

Also, I initially had some trouble getting this to work in my environment. Turns out it was a NODE_MODULE_VERSION issue like the one described here, but it was hard to notice there was even such an error because it was only thrown in the devtools console.

I'm afraid electron-spellchecker will break rather silently like this whenever it's run in a node version it doesn't expect. (Is that a valid concern?) Should we be adding an electron-rebuild step somewhere?

I'm planning to add the same approach using electron-rebuild as we're doing in wp-desktop to check if a rebuild is necessary.
https://github.com/Automattic/wp-desktop/blob/develop/Makefile#L147-L149

Also, I'm planning a PR that is simplifying debugging when we are in dev mode like introducing keyboard shortcuts for showing dev tools, ...

@mirka mirka force-pushed the feature/electron-spellchecker branch from cc5255e to f9de260 Compare September 6, 2018 13:06
@adlk
Copy link
Contributor Author

adlk commented Sep 6, 2018

@mirka @roundhill the required changes were made. Plus,

  1. I've added the make targets:

    • make dev: starting the webpack dev server
    • make start: start electron and check if a native dependency rebuild is necessary
    • make rebuild-deps: rebuilds native dependencies. This is a dependency of make start
  2. Spellchecker is now just used in fields where we really want spell checking to happen.

I'm going to test everything once more and then we should be done here.

@adlk adlk requested a review from mirka September 7, 2018 10:16
@adlk adlk changed the base branch from chore/build-improvements to master September 7, 2018 10:16
@mirka mirka mentioned this pull request Sep 7, 2018
19 tasks

.PHONY: rebuild-deps
rebuild-deps:
@npx electron-rebuild
Copy link
Member

Choose a reason for hiding this comment

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

Worked great 👍

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

The TagList is showing a context menu when it shouldn't, but it was a markup problem (#844) and not a problem with this spellchecker PR.

Works great, thank you!

@adlk adlk merged commit a6ab0f2 into master Sep 13, 2018
@adlk adlk deleted the feature/electron-spellchecker branch September 13, 2018 11:44
@mirka mirka mentioned this pull request Sep 22, 2018
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.

3 participants