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

Editor: Proofreading #3468

Merged
merged 6 commits into from
Mar 14, 2016
Merged

Editor: Proofreading #3468

merged 6 commits into from
Mar 14, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 22, 2016

Closes #306

This pull request seeks to enable a user to proofread their writing via the AfterTheDeadline service.

proofreading

Implementation notes:

The plugin is largely a port of the atd.core.js and editor_plugin.js modules, adapted for use in Calypso.

Main points of divergence include:

  • Revised code styling to conform to Calypso lint rules
  • Communicate with AfterTheDeadline via CORS, not proxy endpoint [1]
  • Persist "Always Ignore Suggestion" options to user preferences [1] [2]
  • Use Calypso lib/mixins/i18n for string translation [1]
  • Add proper default browser spellcheck disabling [1]
  • Use supported localized AfterTheDeadline service hostname if can be determined [1]

Because this is a port of large preexisting files, it's recommended that you focus review on the items listed above. It is not the intention at this time to convert existing files to ES6 standards.

Testing instructions:

Verify that proofreading behaves as expected, particularly with regard to the items listed above in Implementation Notes.

  1. Navigate to the Calypso post editor
  2. Select a site, if prompted
  3. Note that a "Proofread Writing" button is included in the editor toolbar
  4. Enter content in the editor content area, intentionally including a proofreading mistake
  5. Note that your browser built-in spellcheck may flag the spelling mistake
  6. Click the "Proofread Writing" button in the editor toolbar
  7. Note that the spelling mistake is flagged with a solid red underline
  8. Note that if browser built-in spellcheck was flagged in Step 5, it is no longer flagged
  9. Click the spelling mistake
  10. Note that the mistake reason is noted, including substitutions
  11. Click a substitution, Ignore Suggestion, or Ignore Always
  12. Note that...
    • If a substitution is clicked, the text is replaced
    • If Ignore Suggestion is clicked, the text is unflagged as being mistaken
    • If Ignore Always is clicked, the text is unflagged as being mistaken, and upon saving and reloading the page, the error is not flagged on subsequent Proofread Writing checks

Open questions:

  • Should we add a feature flag, restricting this to particular environments pending further testing?

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 22, 2016
@aduth aduth self-assigned this Feb 22, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 24, 2016

I've spent a considerable part of my morning trying to debug why Safari iOS appears to be ignoring spellcheck attributes. Even in the following minimally reproducible page, I'm still encountering spelling suggestions upon tapping words, despite having all of the standard supported spellcheck disabling attributes present:

http://output.jsbin.com/tosebedofa

Currently downloading iOS 9.3 SDK beta in hopes that this may be resolved in the upcoming release.

* atd.core.js - A building block to create a front-end for AtD
* Author : Raphael Mudge & Andrew Duthie, Automattic
* License : LGPL
* Project : http://www.afterthedeadline.com/developers.slp
Copy link
Contributor

Choose a reason for hiding this comment

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

This URL is invalid - I think it should be http://www.afterthedeadline.com/development.slp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This URL is invalid

Good eyes! Fixed in rebased c312168

@aduth
Copy link
Contributor Author

aduth commented Feb 24, 2016

I've pushed some changes which should be a slight improvement of behavior on mobile. Unfortunately, this does not include any fix for the default browser spellcheck in iOS, and the browser spellcheck appears to interfere with the plugin suggestions menu.

Improvements include:

  • Suggestions menu should reposition when editor is scrolled (b0efb4b)
  • Suggestions menu should be dismissed when tapping other words in the editor, and when dismissing the mobile flyup keyboard (699d6aa)
  • Suggestions menu should be dismissed when cancelling proofreading mode (61b872c)

With regards to iOS Safari default browser spellcheck, the workaround I've found is dismissing the keyboard via "Done", then tapping on the next highlighted term.

* Module variables
*/
const SERVICE_HOSTNAME = 'service.afterthedeadline.com';
const SERVICE_LOCALIZED_SUBDOMAINS = [ 'en', 'pt', 'de', 'es', 'fr' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not knowing much about this service at all, should the button only be shown it the user's locale is available here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not knowing much about this service at all, should the button only be shown it the user's locale is available here?

Admittedly I'm following the precedent here, as the Jetpack implementation falls back to the non-localized subdomain when it cannot determine a supported locale.

There's also the question of whether we detect based on the user's locale, or the site's locale? I might imagine that we'd still want to show the option even if the user's specific locale is not supported, as they may be still be writing for an English site.

@aduth
Copy link
Contributor Author

aduth commented Feb 25, 2016

Currently downloading iOS 9.3 SDK beta in hopes that this may be resolved in the upcoming release.

The same issue remains present for me in iOS 9.3 Safari.

@timmyc
Copy link
Contributor

timmyc commented Mar 11, 2016

I took this for a spin on the windows touch device, and it worked well. I'd say we get this in if you are ready to roll with it @aduth

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 11, 2016
aduth added a commit that referenced this pull request Mar 14, 2016
@aduth aduth merged commit 217742f into master Mar 14, 2016
@aduth aduth deleted the add/editor-atd branch March 14, 2016 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants