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

Prevent paired browsing client script from being sanitized #6099

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Apr 23, 2021

Summary

In Gutenberg v9.9 the wp-i18n script is now dependent on the wp-hooks script (see WordPress/gutenberg#27966). This has now caused a validation error to occur for the paired browsing client script as wp-hooks is not apart of the list of dependencies to mark with the data-ampdevmode attribute. This PR fixes that.

Before After
image image

Aside: We may want to give higher priority to #4001 since we didn't immediately catch this.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@pierlon pierlon added Bug Something isn't working WS:Core Work stream for Plugin core labels Apr 23, 2021
@pierlon pierlon added this to the v2.1 milestone Apr 23, 2021
@pierlon pierlon requested a review from westonruter April 23, 2021 04:44
@pierlon pierlon self-assigned this Apr 23, 2021
@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #6099 (d14b3fd) into develop (31e603a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6099      +/-   ##
=============================================
- Coverage      74.91%   74.89%   -0.02%     
  Complexity      5775     5775              
=============================================
  Files            231      231              
  Lines          17487    17491       +4     
=============================================
  Hits           13100    13100              
- Misses          4387     4391       +4     
Flag Coverage Δ Complexity Δ
javascript 79.84% <ø> (ø) 0.00 <ø> (ø)
php 74.67% <100.00%> (-0.02%) 5775.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/Admin/PairedBrowsing.php 100.00% <100.00%> (ø) 29.00 <0.00> (ø)
src/Admin/OptionsMenu.php 30.69% <0.00%> (-1.27%) 18.00% <0.00%> (ø%)
assets/src/components/amp-setting-toggle/index.js 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)

@github-actions
Copy link
Contributor

Plugin builds for d14b3fd are ready 🛎️!

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

I switched the dev environment for amp-wp.org to Transitional mode and confirmed that this validation error is now showing up. This PR does fix the issue.

@westonruter westonruter merged commit cf7b2b3 into develop Apr 23, 2021
@westonruter westonruter deleted the fix/paired-browsing-client-validation-error branch April 23, 2021 17:26
@westonruter westonruter assigned westonruter and unassigned pierlon Apr 23, 2021
@westonruter
Copy link
Member

QA Passed

There is no longer a validation error being detected when a site is in Transitional mode:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants