Prevent loading auto lightbox when disabled using data-amp-auto-lightbox-disable attribute #37854
Conversation
| // Explicitly opted out. | ||
| '[data-amp-auto-lightbox-disable]', | ||
|
|
There was a problem hiding this comment.
Don't we want this documentation here still? How else will people know?
There was a problem hiding this comment.
I removed this line from here because it won't be executed as we are excluding the amp-auto-lightbox.js script if the data-amp-auto-lightbox-disable attribute is present in the body. Can you suggest any .md file where we can document this attribute for people to know?
There was a problem hiding this comment.
This should be left, since it allows individual opt outs without disabling all auto-lightbox.
|
Hey @dethstrobe just following up on this PR. Please let me know if I can help unblock this PR in any way. |
| // Explicitly opted out. | ||
| '[data-amp-auto-lightbox-disable]', | ||
|
|
There was a problem hiding this comment.
This should be left, since it allows individual opt outs without disabling all auto-lightbox.
src/auto-lightbox.js
Outdated
| if ( | ||
| !isAmphtml(win.document) || | ||
| !ampdoc.isSingleDoc() || | ||
| win.document.body.hasAttribute('data-amp-auto-lightbox-disable') |
There was a problem hiding this comment.
When this is called, we can't guarantee that the <body> (or all of its attributes) have bene parsed. It would be safer to put this on the <html> element instead.
There was a problem hiding this comment.
@jridgewell Should it be something like this:
<html ⚡ lang="en" data-amp-auto-lightbox-disable>
...
</html>Please correct me if there is misinterpretation.
There was a problem hiding this comment.
Ok. Let me revise implementation 👍🏻
There was a problem hiding this comment.
@jridgewell Done. I have revised implementation and updated markdown (refer 02ff8d1)
Let me know if there is anything that needs to be taken care of.
Marking `data-amp-auto-lightbox-disable` as `<html ⚡ lang="en" data-amp-auto-lightbox-disable>` will disable `amp-auto-lightbox` from auto-load on page load.
…y` to `html` This will check if HTML Tag has `data-amp-auto-lightbox-disable` attribute like: `<html ⚡ lang="en" data-amp-auto-lightbox-disable>`. If it has, it will disable auto-loading of `amp-auto-lightbox` on page load.
| @@ -241,10 +241,6 @@ describes.realWin( | |||
| accepts: 'elements inside non-clickable anchor', | |||
| wrapWith: () => html` <a id="my-anchor"></a> `, | |||
| }, | |||
| { | |||
| rejects: 'explicitly opted-out subnodes', | |||
| mutate: (el) => el.setAttribute('data-amp-auto-lightbox-disable', ''), | |||
|
So adding |
|
I ask because the AMP plugin now is adding |
|
I believe you only need to add it to the |
Fixes #26452
data-amp-auto-lightbox-disableattribute is added to the body tag