Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

AMP compatibility mode #48

Closed
pewgeuges opened this issue Feb 25, 2021 · 10 comments
Closed

AMP compatibility mode #48

pewgeuges opened this issue Feb 25, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@pewgeuges
Copy link
Contributor

pewgeuges commented Feb 25, 2021

After adding hard links in 2.3.0 (2020-12-31), Footnotes is now enabled to become fully AMP compatible thanks to @milindmore22 code contribution and @westonruter code contribution. This release is now fast-tracked and shall be followed by submission to AMP PROJECT / WordPress for testing.

[Description restored to its original state. Please see #issuecomment-797859947 for further information.]

@lolzim
Copy link
Contributor

lolzim commented Mar 12, 2021

\o/

@pewgeuges
Copy link
Contributor Author

Yet CSS tooltips are only part of the solution. The plugin also needs AMP script to expand (and collapse) the reference container, because this is triggered mainly by the first click on a footnote referrer.

Moreover, two bugs have been reported shortly after, requiring bugfix releases 2.5.7 (2021-02-27) and 2.5.8 (2021-02-28). As stated in #50: “The plugin is still so buggy it would be shameless to submit it to AMP-WP as-is even after adding AMP compat.”

@westonruter
Copy link

The plugin also needs AMP script to expand (and collapse) the reference container, because this is triggered mainly by the first click on a footnote referrer.

You may not need to use amp-script for this. You could alternatively use amp-bind which would be more lightweight. Another option would be the toggleClass or toggleVisibility actions.

@westonruter
Copy link

A pure CSS-based solution could also be to make use of the :target selector, but I'm not clear exactly what is being described here.

@pewgeuges
Copy link
Contributor Author

pewgeuges commented Mar 15, 2021

@westonruter,

The plugin also needs AMP script to expand (and collapse) the reference container, because this is triggered mainly by the first click on a footnote referrer.

You may not need to use amp-script for this. You could alternatively use amp-bind which would be more lightweight. Another option would be the toggleClass or toggleVisibility actions.

Thank you; toggleClass will probably achieve the effect, because we need to toggle between display: none and display: inline for the footnotes list to completely disappear while collapsed.

A pure CSS-based solution could also be to make use of the :target selector, but I'm not clear exactly what is being described here.

The target is a single footnote number in the list, that is being scrolled to and I see we can have smooth scrolling with the scrollTo, only that the scroll offset position would need to be an integer too. This works with hard links, perhaps we could combine the two.

@milindmore22
Copy link

It seems the AMP attributes are getting added to non-AMP pages too, in case the site has transitional or reader mode user may find some issues eg: check "References" on non-AMP page

This can be easily fixed by doing additional check in MCI_Footnotes->register_public

if ( did_action( 'wp' ) && function_exists( 'is_amp_endpoint' ) ) {
	self::$a_bool_amp_enabled = is_amp_endpoint();
}

I didn't check the whole codebase but can you please let me know why we need an AMP compatibility checkbox and why can't we conditionally check for the AMP endpoint instead as most of plugins and themes do.

image

Rumperuu pushed a commit that referenced this issue Apr 10, 2021
Hopefully we’ll meet the looming deadline.
Thanks for testing and I’ll be reaching out to AMP-WP, in issue #48 to begin with.

git-svn-id: https://plugins.svn.wordpress.org/footnotes/trunk@2497474 b8457f37-d9ea-0310-8a92-e5e31aec5664
Rumperuu pushed a commit that referenced this issue Apr 10, 2021
Bugs are now fixed except fade-in/fade-out.
Hopefully we’ll meet the looming deadline.
Thanks for testing and I’ll be reaching out to AMP-WP, in issue #48 to begin with.

git-svn-id: https://plugins.svn.wordpress.org/footnotes/trunk@2497475 b8457f37-d9ea-0310-8a92-e5e31aec5664
@markcheret
Copy link
Owner

this seems to be ready. Can someone please confirm?

@Rumperuu
Copy link
Collaborator

Rumperuu commented Apr 19, 2021

this seems to be ready. Can someone please confirm?

AFAICT, Pew has implemented the toggleClass suggestion from #48 (comment) (see /class/task.php:1977–1982), but not the endpoint detection suggestion from #48 (comment) yet. I also suspect that #87 will result in changes that affect this. It may be worth parking this until after that's done, and then resubmitting the updated codebase to the AMP-WP team for re-review?

However, I'm unfortunately not able to contribute further on the AMP compat for ethical reasons.

@markcheret
Copy link
Owner

I'm honestly a fan of optimising load times and prioritising user experience. Apart from that, I opt for parking it and it's also ethically highly questionable. I can't remember asking for AMP compatibility or any of the users.
The ethical drawbacks and technical restrictions and debt introduced aren't worth a lightning bolt on the SERPs, though :)

@Rumperuu Rumperuu removed this from the 2.7.1 milestone Apr 19, 2021
@Rumperuu Rumperuu added the wontfix This will not be worked on label Apr 19, 2021
@pewgeuges
Copy link
Contributor Author

@milindmore22 Excuse me please for failing to respond to your question #48 (comment)

I missed the point of the AMP check.

Unintentionally I got this plugin messed up.

Then I got uneasy with this plugin and pulled out.

Now it seems unfixable since 1 week ago I opened a PR #250 as @markcheret invited me to do when I left, but @markcheret does not respond any longer. @markcheret must be waiting for somebody else.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants