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

Disable autocorrect feature by default #72

Merged
merged 26 commits into from
Oct 19, 2022
Merged

Disable autocorrect feature by default #72

merged 26 commits into from
Oct 19, 2022

Conversation

tdulcet
Copy link
Collaborator

@tdulcet tdulcet commented Jun 6, 2022

I believe this should finally allow us release v1.0. The Unicode autocorrection feature could then be enabled in a future release, such as v2.0.

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM quite much.

  • need to test this in practice I guess

src/options/modules/CustomOptionTriggers.js Outdated Show resolved Hide resolved
src/background/modules/AutocorrectHandler.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
tdulcet and others added 2 commits July 28, 2022 02:33
@tdulcet tdulcet requested a review from rugk July 28, 2022 09:41
@@ -40,7 +24,8 @@ function notification(title, message) {
*/
function fallback(text, fieldId) {
navigator.clipboard.writeText(text);
notification(`📋 Press ${pasteSymbol}-V`, `Add-ons in Thunderbird are currently unable to access the “${fieldId.startsWith("compose") ? fieldId.slice("compose".length) : fieldId}” field directly, so the transformed text has been copied to your clipboard.\nPlease press ${pasteSymbol}-V to do the transformation.`);
// This will need to be localized
Copy link
Owner

Choose a reason for hiding this comment

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

Feel free to add todos as real TODO comments. AFAIK I also I had a not or so which would notify you in this or if you have such a one.

Copy link
Owner

Choose a reason for hiding this comment

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

Of course better fix it directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, will do... I will leave the localization to you.

@tdulcet tdulcet requested a review from rugk July 29, 2022 09:56
src/options/options.html Outdated Show resolved Hide resolved
@rugk
Copy link
Owner

rugk commented Aug 22, 2022

Localized:
image

rugk added a commit to TinyWebEx/CommonCss that referenced this pull request Aug 22, 2022
Tested with Awesome Emoji Picker manually

And Unicodify, needed for rugk/unicodify#72
@rugk
Copy link
Owner

rugk commented Aug 22, 2022

So needed some CSS adjustments: TinyWebEx/CommonCss#2

looks good now with a proper warning message:
image

@rugk
Copy link
Owner

rugk commented Aug 22, 2022

I also simply removed the confirm message for now. I mean, if one really skips the big yellow warning then so be it. It's not that bad. It does not kill the computer or have other non-reversible effects after all.
They can just revert the setting and are fine. Also, it may be useful if you do not use one of the sites affected.

Also it was not really a great UI to have that default Firefox dialog somewhere in here, and worse, if you showed that popup multiple times, it tried to show a "do not disturb" checkbox, but the checkbox is not there… 😅 😅

image

@rugk
Copy link
Owner

rugk commented Aug 22, 2022

requesting review from @tdulcet (cannot do via GitHub as you've opened this PR 😅 )

Copy link
Collaborator Author

@tdulcet tdulcet left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of the localization! Also, thanks for adding the proper warning message, I did not realize we could do that. Anyway, it looks good, just a minor change needed...

It's not that bad. It does not kill the computer or have other non-reversible effects after all.

It does of course completely break Twitter, Facebook and likely other websites and I did not want to get a bunch of negative reviews. However, I agree that the UI was not the best, particularly because it does not currently work on Thunderbird (Bug 1780977) or Chromium (Bug 476350), so I am fine with removing it.

and worse, if you showed that popup multiple times, it tried to show a "do not disturb" checkbox, but the checkbox is not there… 😅 😅

Ha, it looks like you found a Firefox bug. I am able to reproduce it as well.


Edit: I think you forgot to delete the src/common/variables.css file when you added your CommonCSS module.

src/_locales/en/messages.json Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/options/options.html Outdated Show resolved Hide resolved
@tdulcet tdulcet requested a review from rugk September 6, 2022 11:43
rugk
rugk previously approved these changes Oct 19, 2022
src/_locales/en/messages.json Outdated Show resolved Hide resolved
src/options/options.html Outdated Show resolved Hide resolved
@rugk
Copy link
Owner

rugk commented Oct 19, 2022

Ha, it looks like you found a Firefox bug. I am able to reproduce it as well.

BTW, if you want to report that bug, feel free… 🙃

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