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

Proposed fix for #404 #628

Closed
wants to merge 1 commit into from
Closed

Proposed fix for #404 #628

wants to merge 1 commit into from

Conversation

soatok
Copy link

@soatok soatok commented Nov 13, 2020

All Submissions:

Changes proposed in this Pull Request:

Closes #404 .

How to test the changes in this Pull Request:

  1. Create a blog with more than 30 test posts, with 10 displayed in the list block.
  2. Verify that you can press the button more than twice without the block disappearing.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo
Copy link
Contributor

dkoo commented Jan 11, 2021

@soatok Thanks for the proposed fix—we'll review and test this shortly.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

@soatok Thanks for the proposed fix! I believe you've correctly identified the cause of the issue here, but there are a few things we'd want to implement in a fix:

  1. We'd need to implement the fix in both AMP JS (the file you've edited here) and non-AMP JS (/src/blocks/homepage-articles/view.js). You can test the non-AMP JS by disabling AMP on the particular page, by deactivating the AMP plugin, or setting the plugin to Transitional mode and viewing the base permalink (without /amp suffix).
  2. We'd like to refactor the AMP JS to avoid the use of new URL, which is not supported by IE11 or below. This would be an improvement over our current code, which uses the URL class in AMP JS while relying on plain strings in non-AMP JS.
  3. We'd need to ensure that the custom REST route still works correctly if not passed the exclude_ids param, or if the param is an empty array. So this might mean selectively retaining the page param and/or fixing its implementation on the PHP side.

Would you like to try to implement the above? If not, we're happy take over this fix (with props to you for identifying the solution). Thanks again!

@soatok
Copy link
Author

soatok commented Jan 15, 2021

Would you like to try to implement the above? If not, we're happy take over this fix (with props to you for identifying the solution). Thanks again!

Unfortunately I'm a bit triple-booked at work right now. Please feel free to take over this fix. Thanks!

@dkoo dkoo mentioned this pull request Jan 18, 2021
6 tasks
@dkoo
Copy link
Contributor

dkoo commented Jan 18, 2021

Rather than messing with the fork, I'm closing this PR in favor of #672.

@dkoo dkoo closed this Jan 18, 2021
@soatok soatok deleted the patch-1 branch January 20, 2021 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blog Posts Block: Load More Posts button doesn't appear even though there are more posts to load
2 participants