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 built-in support for spellchecking #94

Merged
merged 16 commits into from Apr 14, 2020
Merged

Add built-in support for spellchecking #94

merged 16 commits into from Apr 14, 2020

Conversation

nautatva
Copy link
Contributor

@nautatva nautatva commented Feb 5, 2020

Built in support for spell check suggestions. This fixes #11.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@nautatva
Copy link
Contributor Author

nautatva commented Feb 5, 2020

This will also solve sindresorhus/caprine#14 which was due and waiting for this PR.

fixture.js Outdated Show resolved Hide resolved
fixture-menu.js Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Make sure it looks exactly like: #11 (comment) I think you've missed some things.

@sindresorhus
Copy link
Owner

This issue has a high bounty, so it's expected that you put a lot of effort into this PR ;)

@bigshahan
Copy link

Seems like the nodeIntegration: true is needed for the searchWithGoogle option being added. Would it be possible to implement a callback alternative for the menu option, and to avoid the require call? I think it is very important to have a workaround for the nodeIntegration requirement, as there is no way we would ever turn that on in our usage, for security reasons.

@nautatva
Copy link
Contributor Author

  • Spelling suggestions
  • Ignore spelling
  • Correct spelling Automatically
  • Learn spelling
  • search with google

Implemented these things in this commit as from the figure in #11 (comment).

The bottom-most options seem more of a system-wide setting change rather than a app-context. Not sure if we can implement this on a non-system app. Even MS-word doesn't have these options.
I was stuck at implementing the ignore spelling part. Can you help me with that please @sindresorhus

@nautatva
Copy link
Contributor Author

I re-checked the searchWithGoogle option and it works even without nodeIntegration: true. Can you recheck if something else isn't being missed out.

@bigshahan
Copy link

I re-checked the searchWithGoogle option and it works even without nodeIntegration: true. Can you recheck if something else isn't being missed out.

This makes sense, since the context menu is setup in the main process.

@sindresorhus
Copy link
Owner

The bottom-most options seem more of a system-wide setting change rather than a app-context.

Yup. Those are not relevant here. And not possible to implement.

@sindresorhus
Copy link
Owner

I was stuck at implementing the ignore spelling part. Can you help me with that please @sindresorhus

What are you stuck with specifically? What have you tried?

index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Don't forget to document all the menu items and options, in both the readme and the TypeScript file. I see some missing things and inconsistencies. The readme and TypeScript file should be in sync.

@nautatva
Copy link
Contributor Author

Had opened a electron issue for the same: electron/electron#22362. I think ignore should be out of question here.

@nautatva
Copy link
Contributor Author

nautatva commented Mar 4, 2020

Any updates for changes? @sindresorhus

@sindresorhus
Copy link
Owner

@sindresorhus
Copy link
Owner

#94 (comment) is not fixed.

@sindresorhus
Copy link
Owner

#94 (comment) is not fixed.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Please spend some more time improving the docs and perfecting this PR :)

@nautatva
Copy link
Contributor Author

Have a look at the changes in this commit

Docs

spellcheck needed documented

  • readme
  • typescript

showSearchWithGoogle

  • readme
  • typescript

searchWithGoogle

  • readme
  • typescript

spellCheck

  • readme
  • typescript

correctAutomatically

  • readme
  • typescript

learnSpelling

  • readme
  • typescript

Other changes

  • used array#map instead of for loop
  • encoding the url

index.js Outdated Show resolved Hide resolved
@nautatva
Copy link
Contributor Author

Is this okay? Haven't work with this before.

index.js Outdated
const url = new URL('https://www.google.com/search?q=' + props.selectionText).href;
const queryParams = {q: props.selectionText};
const queryString = new URLSearchParams(queryParams).toString();
const url = new URL('https://www.google.com/search?' + queryString).href;
Copy link
Owner

Choose a reason for hiding this comment

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

This is not what I asked for. I asked you to use URL#searchParams. const url = new URL(…); url.searchParams.set(…);.

Copy link
Owner

Choose a reason for hiding this comment

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

And use url.toString().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this in the following commit, but am curious, why can't we use const url = new URL('https://www.google.com/search?' + new URLSearchParams(queryParams).toString()).href;

Is it just because of readability or it due to some other reason as well?

Copy link
Owner

Choose a reason for hiding this comment

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

You technically could, but what if you forget to put the ? at the end. I prefer using dedicated APIs for formatting whenever possible to prevent mistakes.

@nautatva
Copy link
Contributor Author

Does this work?

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Feature:Builtin support for spellcheck suggestions Add built-in support for spellchecking Apr 14, 2020
@sindresorhus sindresorhus merged commit 71c5d2e into sindresorhus:master Apr 14, 2020
@sindresorhus
Copy link
Owner

Yup, looks good now 👍

@sindresorhus
Copy link
Owner

@nautatva Can you do a PR to Caprine now to get the bounty there too?

@nautatva
Copy link
Contributor Author

Done!

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.

Built in support for spell check suggestions
4 participants