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

Fix block and page-level validation notices #1676

Merged
merged 9 commits into from Nov 30, 2018

Conversation

@kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 29, 2018

Before

before-validation

After

accepted-notice

Fixes #1502.

kienstra added 2 commits Nov 29, 2018
…ed (not 'New Accepted')

As Weston mentioned,
the block-level validation notices didn't appear if
the error was 'New Accepted'.
So display the error for these statuses:
- 'New Accepted
- 'New Rejected'
- 'Rejected'
and don't display for:
- 'Accepted'
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 29, 2018

As @westonruter mentioned, the block-level validation notices didn't appear if the error was 'New Accepted'.

So this displays a block-level notice for these statuses:

  • 'New Accepted
  • 'New Rejected'
  • 'Rejected'

...and doesn't display a notice for:

  • 'Accepted'
@@ -171,8 +171,7 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
return (
0 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS */ === result.status ||
1 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS */ === result.status ||
2 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS */ === result.status || // eslint-disable-line no-magic-numbers
3 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS */ === result.status // eslint-disable-line no-magic-numbers
2 /* \AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS */ === result.status // eslint-disable-line no-magic-numbers

This comment has been minimized.

@kienstra

kienstra Nov 30, 2018
Author Contributor

Nice, thanks for fixing this.

… in #1502

Display different messages when errors are forcibly accepted.
Mainly use Weston's copy from #1502.
Also, if there aren't any 'Rejected' or 'New Rejected' errors,
display the notice confirming that.
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

Paired mode with auto-sanitize on, or ‘Native’ mode

paired-auto-sanitize-or-native

Paired mode with ‘auto_accept_sanitization’ true, but there are still non-accepted errors

auto-accept-with-rejected

Paired or Classic mode, auto-accept disabled (unchanged in this PR)

paired-no-auto-accept

@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

Question About Commit Above

Hi @westonruter,
Does 4fea78b apply your point in #1502 (comment)?

I'm not sure if it captures what you'd like. The comment above shows the main cases it covers.

Of course, I'm also working on applying this to the block editor.

Thanks, Weston!

… message

In that case, it will block this from serving AMP.
So display the notice about the unaccepted error(s).
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

With the latest commit (5afdc3e):

Native mode, only ‘Accepted’ and ‘New Accepted’ errors

native-mode-only-accepted-errors

Native mode, with ‘Rejected’ or ‘New Rejected’ errors

native-rejected-errors

kienstra added 2 commits Nov 30, 2018
If amp_is_canonical(),
self::is_sanitization_auto_accepted() will also be true.
So it's no need to check that separately.
Use similar logic to determine the notices to
that in the Classic editor.
But the copy is different,
because the block editor copy has a count of the validation errors,
and those that are from the post content.
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

Block Editor Copy

Native mode with only ‘Accepted’ or ‘New Accepted’ validation errors

native-mode-only-accepted

Native Mode with a ‘Rejected’ or ‘New Rejected’ error

(Or Paired Mode, with auto-accept, with a ‘Rejected’ or ‘New Rejected’ error)
native-mode-rejected-new-rejected

Paired mode with auto-accept disabled, with any validation error other than ‘Accepted’ (including ‘New Accepted’)

This isn't changed in this PR.
paired-mode-no-auto-accept

Instead of calling self::is_sanitization_auto_accepted() twice,
simply create an 'if' and 'else' block inside it.
@kienstra kienstra changed the title [WIP] Fix block and page-level validation notices Fix block and page-level validation notices Nov 30, 2018
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

Request For Review

Hi @westonruter,
Could you please review this? The 2 comments above have screenshots of the Classic and block editors.

This might not be what you had in mind on #1502 (comment).

Thanks, Weston!

@westonruter westonruter added this to the v1.0 milestone Nov 30, 2018
@westonruter
Copy link
Member

@westonruter westonruter commented Nov 30, 2018

@kienstra Overall this is looking great. I have found an issue where if there are accepted validation errors, the notice is still being displayed unexpectedly (also mentioning 0 errors):

image

@westonruter
Copy link
Member

@westonruter westonruter commented Nov 30, 2018

I can reproduce this behavior if I:

  1. Add some invalid markup.
  2. Save draft.
  3. Click "Review issues" link to open in new tab.
  4. Accept the validation error and save the validated URL post.
  5. Make another change to the post and save another draft.
  6. At this point the notice is shown saying there are 0 validation errors needing to be reviewed.
@westonruter westonruter merged commit 895c731 into 1.0 Nov 30, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@westonruter westonruter deleted the update/block-level-validation branch Nov 30, 2018
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Nov 30, 2018

Hi @westonruter, thanks a lot for fixing that issue with ee95ae3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.