-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
This will also solve sindresorhus/caprine#14 which was due and waiting for this PR. |
Make sure it looks exactly like: #11 (comment) I think you've missed some things. |
This issue has a high bounty, so it's expected that you put a lot of effort into this PR ;) |
Seems like the |
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 re-checked the |
This makes sense, since the context menu is setup in the main process. |
Yup. Those are not relevant here. And not possible to implement. |
What are you stuck with specifically? What have you tried? |
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. |
Had opened a electron issue for the same: electron/electron#22362. I think ignore should be out of question here. |
Any updates for changes? @sindresorhus |
#94 (comment) is not fixed. |
#94 (comment) is not fixed. |
Please spend some more time improving the docs and perfecting this PR :) |
Have a look at the changes in this commit Docsspellcheck needed documented
showSearchWithGoogle
searchWithGoogle
spellCheck
correctAutomatically
learnSpelling
Other changes
|
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; |
There was a problem hiding this comment.
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(…);
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use url.toString()
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Does this work? |
Yup, looks good now 👍 |
@nautatva Can you do a PR to Caprine now to get the bounty there too? |
Done! |
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