Skip to content

Add comment sorting support#1024

Merged
westonruter merged 2 commits into0.7from
add/comment-sorting-support
Apr 2, 2018
Merged

Add comment sorting support#1024
westonruter merged 2 commits into0.7from
add/comment-sorting-support

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Mar 17, 2018

Depends on ampproject/amphtml#5396 which should be deployed to production in the next couple weeks (when experiment flag is disabled: ampproject/amphtml#13552). This is something that should be part of 0.7.

@westonruter
Copy link
Copy Markdown
Member Author

I've deployed the changes to https://commentsort-ampconfdemo.pantheonsite.io/2018/02/06/are-you-there-lebron-its-me-lebron-a-superstars-ultimate-pep-talk/ (with re-generated spec not yet pushed to this branch).

However, it doesn't seem to be working: https://youtu.be/1Zh7L9_uI6k

* Add maps URL protocol.
* Allow referrerpolicy in links.
* Allow expand-single-section on amp-accordion.
* Allow template attribute on amp-ad and amp-embed.
* Add amp-beopinion component.
* Add amp-bodymovin-animation component.
* Allow lightbox attributes on amp-carousel, amp-img, amp-video, amp-youtube.
* Add amp-document-recommendations component.
* Allow amp-ima-video@autoplay.
* Allow amp-list to have amp-bind [src].
* Add amp-live-list@sort.
* Add amp-story components: amp-story-cta-layer, amp-story-auto-ads.
* Add blacklist value regex to link@rel.
* Add 'link rel=preload'.
* Update regex for 'link rel=stylesheet for fonts'.
* Add 'meta name=amp-to-amp-navigation'.
* Add amp-subscriptions.
@westonruter
Copy link
Copy Markdown
Member Author

It seems to test you have to both enable dev-channel and amp-live-list-sorting experiments on https://cdn.ampproject.org/experiments.html and then also toggle the experiments via the AMP.toggleExperiment() calls in the console. Once this is done, it works as expected!

@westonruter westonruter changed the title Add comment sorting support [blocked] Add comment sorting support Mar 20, 2018
@westonruter westonruter changed the title [blocked] Add comment sorting support Add comment sorting support Apr 2, 2018
@westonruter westonruter requested a review from kienstra April 2, 2018 18:53
@westonruter
Copy link
Copy Markdown
Member Author

@kienstra This is live now (and will be in validator shortly): ampproject/amphtml#5396 (comment)

So it can be merged and tested, along with the change to the ampconf theme: xwp/ampnews#84

@kienstra kienstra self-assigned this Apr 2, 2018
Copy link
Copy Markdown
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
This PR looks good, and it works well locally with the AMPConf theme PR #84.

Could you please merge this? I would, but I don't have permissions.

most-recent-bottom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants