-
Notifications
You must be signed in to change notification settings - Fork 381
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
Hide AMP Noloading toggle if block does not have specified setting #5574
Hide AMP Noloading toggle if block does not have specified setting #5574
Conversation
Plugin builds for 8a6d522 are ready 🛎️!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion on this, @pierlon
* @param {Object} props Props. | ||
* | ||
* @return {ReactElement} Element. | ||
*/ | ||
const AmpNoloadingToggle = ( props ) => { | ||
const { attributes: { ampNoLoading }, setAttributes } = props; | ||
|
||
if ( ! ampNoLoading ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be:
if ( ! ampNoLoading ) { | |
if ( undefined === ampNoLoading ) { |
If it's just ! ampNoLoading
, then the toggle will be hidden on existing blocks that have it set to false. And if an existing block has it set to true and it is toggled to false, the toggle will disappear. That's not what we intend, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that's true, nice catch. I've addressed that in b390973.
8832470
to
b390973
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5574 +/- ##
=============================================
+ Coverage 73.62% 73.64% +0.02%
Complexity 5424 5424
=============================================
Files 187 187
Lines 16364 16366 +2
=============================================
+ Hits 12048 12053 +5
+ Misses 4316 4313 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in #5573 (review), please add a deprecation notice that warns the feature will be removed.
<span dangerouslySetInnerHTML={ { | ||
__html: sprintf( | ||
/* translators: placeholder is link to support forum. */ | ||
__( 'The AMP Layout setting is deprecated and is slated for removal. Please <a href="%s" target="_blank" rel="noreferrer">report</a> if you need it.', 'amp' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be copy/pasta from #5573. Shouldn't it reference “AMP Noloading” and be put on AmpNoloadingToggle
below instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should 🤦. Resolved in d8a4e0c.
d358e75
to
d8a4e0c
Compare
…nt/4555-deprecate-amp-noloading-setting * 'develop' of github.com:ampproject/amp-wp: Add a warning notice to setting Add force flag to rm on the productionVendorExcludedFilePatterns Bump ampproject/amp-toolbox from 0.1.0 to 0.1.1 Rename test file Test AmpLayoutControl Use stricter condition check Hide AMP Layout toggle if block does not have specified setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
Fixes #4555
Checklist