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

Add comment sorting support #1024

Merged
merged 2 commits into from Apr 2, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Collaborator

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.

  • Remove forced descending comment order
  • Update ampconf theme to add sort=ascending to amp-live-list (see xwp/ampnews#84).
  • Re-generate from spec to add recognition for sort=ascending attribute.
  • Deploy to test environment and enable experiment to test via adding the following to the console: AMP.toggleExperiment('amp-live-list-sorting') (to check if it is enabled, run AMP.isExperimentOn('amp-live-list-sorting'))
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Mar 17, 2018

Collaborator

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

Collaborator

westonruter commented Mar 17, 2018

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

Update generated spec data from revision 566 to 586
* 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

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Mar 17, 2018

Collaborator

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!

Collaborator

westonruter commented Mar 17, 2018

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 from Add comment sorting support to [blocked] Add comment sorting support Mar 20, 2018

@westonruter westonruter changed the title from [blocked] Add comment sorting support to Add comment sorting support Apr 2, 2018

@westonruter westonruter requested a review from kienstra Apr 2, 2018

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Apr 2, 2018

Collaborator

@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

Collaborator

westonruter commented Apr 2, 2018

@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

@kienstra

kienstra approved these changes Apr 2, 2018 edited

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

@kienstra kienstra assigned westonruter and unassigned kienstra Apr 2, 2018

@westonruter westonruter merged commit 49f5928 into 0.7 Apr 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/comment-sorting-support branch Apr 2, 2018

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