🚑 Quick fix prevent grammarly from running on chromium based browsers #13820

Link2Twenty · 2021-05-20T17:46:53Z

What type of PR is this? (check all applicable)

Description

Grammarly was preventing users from writing posts, while using chromium, this patch prevents grammarly from running in chromium browsers

Related Tickets & Documents

partial #13814

QA Instructions, Screenshots, Recordings

N/A

UI accessibility concerns?

Grammarly is a useful tool for dyslexic people if we can get it working we should

Added tests?

Yes
No, and this is why: old tests will still work
I need help with writing tests

[optional] Are there any post deployment tasks we need to perform?

N/A

[optional] What gif best describes this PR or how it makes you feel?

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

github-actions · 2021-05-20T17:47:08Z

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

nickytonline

@Link2Twenty, thanks so much for the fix. I have a different approach that builds off of what you've done which I mention in the comments.

Looking forward to seeing this in production!

nickytonline · 2021-05-20T22:56:45Z

app/javascript/crayons/MentionAutocompleteTextArea/MentionAutocompleteTextArea.jsx

@@ -391,6 +392,7 @@ export const MentionAutocompleteTextArea = forwardRef(
                <ComboboxList>
                  {users.map((user) => (
                    <ComboboxOption
+                      key={`${user.username}_key`}


Nitpick (non-blocking): Adding _key isn't necessary. They key as user.username is already unique and not related to this fix.

nickytonline · 2021-05-20T23:00:21Z

app/javascript/article-form/components/EditorBody.jsx

@@ -43,6 +43,7 @@ export const EditorBody = ({
  onChange,
  defaultValue,
  switchHelpContext,
+  disableGrammarly,


Refactor (blocking): Create a function for the Grammarly data attribute and avoid prop drilling.

There are two spots where we need to disable Grammarly. The title and body of a post in the editor and hopefully this code can be removed in the near future (assuming Grammarly fixes whatever they changed or implements it in a different manner). As well, we don't want the <MentionAutoCompleteTextArea /> to be aware of Grammarly. We can just pass in the additional props and spread them.

It would take too long to comment on the PR so I created a branch to demonstrate what I'm proposing. See https://github.com/forem/forem/tree/nickytonline/poc-disable-grammarly.

In my linked POC branch I made it a hook, but it just needs to be a function, not a hook.

There's a third place we need to disable it too - the connect chat box is showing the same issues

nickytonline · 2021-05-20T23:03:46Z

app/javascript/article-form/components/Form.jsx

@@ -18,12 +18,16 @@ export const Form = ({
  switchHelpContext,
  errors,
 }) => {
+  // 🚑 Detect if user is using chromium to disable grammarly
+  const isChrome = !!window.chrome;


This check will also return true for all Chromium-based browsers which we don't want as Grammarly currently works in at least Edge (Chromium). We could combine this check with a regex test on the user-agent, e.g.

!/(Chrome\/.+Edg\/)|/.test(navigator.userAgent)

I haven't checked the Brave user-agent or others, but we could add them in the regex.

I think that regex may be incorrect

Hmm I tested it yesterday in Chrome, Edge, and Firefox and it seem to behave properly. I'm using .+ in the middle as there is a lot in the user-agent string. I can help with this if it's blocking you.

aitchiss

I gave this a quick spin and it seems to all be working as hoped, I just had a few very minor change requests but otherwise looks great 👍 Thanks for taking this on!

app/javascript/utilities/useDisableGrammarly.js

app/javascript/crayons/MentionAutocompleteTextArea/MentionAutocompleteTextArea.jsx

…completeTextArea.jsx Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

aitchiss · 2021-05-24T13:36:54Z

Hey @Link2Twenty - something we've just become aware of is that even when Grammarly appears to "work" in Firefox / non-Chrome browsers, it's not consistently allowing insertion of Grammarly's suggestions, and is just generally causing weirdness.

We missed that before, but given it's behaving in a 'broken' way on all browsers, it's probably best that we simplify this PR and just add the relevant attribute inline to the various textareas (without the need to check if we're in Chrome / have a separate function).

Sorry for the confusion - it seems we all tested that Grammarly was making suggestions/highlighting, but we didn't check that suggestions were being inserted correctly (woops 😅 )

Link2Twenty · 2021-05-24T14:04:08Z

These things happen 😁
I've changed everything over to just being hard coded now.

aitchiss

Tested this out locally on a few browsers and all worked OK despite having Grammarly running.

We'll need to keep an eye on this / explore more under #13814 as Grammarly is such a useful tool and we definitely only want this to be a temporary patch!

nickytonline

Thanks for the fix @Link2Twenty! LGTM! 🛳

Link2Twenty and others added 13 commits March 17, 2021 14:31

Update with best practices

10a83fc

Remove useCallback

bd8af0b

Update app/javascript/shared/components/useKeyboardShortcuts.js

1e091a8

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

Fill out JSDoc

b0e9f56

Follow Prettier rules

f5a4f65

Resolve conflict

3c51291

Merge branch 'master' into master

96b4cfd

Merge remote-tracking branch 'upstream/master'

f506a42

Merge branch 'forem:main' into main

cad0dcd

Merge branch 'forem:main' into main

8a03cae

Merge branch 'forem:main' into main

aa07258

Merge branch 'forem:main' into main

df0a639

🚑 Disable grammarly on chromium

6a5980c

Link2Twenty requested a review from a team May 20, 2021 17:46

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label May 20, 2021

Link2Twenty requested review from nickytonline and jacobherrington and removed request for a team May 20, 2021 17:46

juliannatetreault requested a review from aitchiss May 20, 2021 19:31

nickytonline added a commit that referenced this pull request May 20, 2021

Suggested tweaks for #13820

71f4472

nickytonline suggested changes May 20, 2021

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels May 20, 2021

♻️ Add new useDisableGrammarly hook

b7f08d1

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels May 21, 2021

💬 remove _key from keyname

db80108

aitchiss suggested changes May 24, 2021

View reviewed changes

pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label May 24, 2021

pr-triage bot added the PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes label May 24, 2021

Update app/javascript/crayons/MentionAutocompleteTextArea/MentionAuto…

90068c0

…completeTextArea.jsx Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels May 24, 2021

Link2Twenty and others added 3 commits May 24, 2021 13:57

Update app/javascript/utilities/useDisableGrammarly.js

deb9958

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

📝 More descriptive function name

84623f2

📝 More descriptive function name

dc4ada3

🚑 Prevent grammarly from running

cbf041f

aitchiss approved these changes May 24, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels May 24, 2021

Link2Twenty requested a review from nickytonline May 25, 2021 08:27

nickytonline approved these changes May 25, 2021

View reviewed changes

nickytonline merged commit 61cd4b3 into forem:main May 25, 2021

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚑 Quick fix prevent grammarly from running on chromium based browsers #13820

🚑 Quick fix prevent grammarly from running on chromium based browsers #13820

Link2Twenty commented May 20, 2021

github-actions bot commented May 20, 2021

nickytonline left a comment •

edited

nickytonline May 20, 2021

nickytonline May 20, 2021 •

edited

aitchiss May 21, 2021

nickytonline May 20, 2021

Link2Twenty May 21, 2021

nickytonline May 21, 2021

aitchiss left a comment

aitchiss commented May 24, 2021

Link2Twenty commented May 24, 2021

aitchiss left a comment

nickytonline left a comment

🚑 Quick fix prevent grammarly from running on chromium based browsers #13820

🚑 Quick fix prevent grammarly from running on chromium based browsers #13820

Conversation

Link2Twenty commented May 20, 2021

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added tests?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions bot commented May 20, 2021

nickytonline left a comment • edited

Choose a reason for hiding this comment

nickytonline May 20, 2021

Choose a reason for hiding this comment

nickytonline May 20, 2021 • edited

Choose a reason for hiding this comment

aitchiss May 21, 2021

Choose a reason for hiding this comment

nickytonline May 20, 2021

Choose a reason for hiding this comment

Link2Twenty May 21, 2021

Choose a reason for hiding this comment

nickytonline May 21, 2021

Choose a reason for hiding this comment

aitchiss left a comment

Choose a reason for hiding this comment

aitchiss commented May 24, 2021

Link2Twenty commented May 24, 2021

aitchiss left a comment

Choose a reason for hiding this comment

nickytonline left a comment

Choose a reason for hiding this comment

nickytonline left a comment •

edited

nickytonline May 20, 2021 •

edited