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

amp-list: Attribute to control amp-bind interaction #16833

Merged
merged 13 commits into from Jul 20, 2018

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Jul 17, 2018

Partial for #15272.

Add new amp-list attribute binding="always|refresh|no" to control whether rendered children are scanned for bindings and evaluated.

  • always is default and status quo.
  • refresh is same as always except it skips the initial render.
  • no disables this functionality entirely.

TODO: Documentation updates.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation lgtm after value change

@@ -42,6 +42,10 @@ tags: { # <amp-list> with mandatory src attr
html_format: EXPERIMENTAL
tag_name: "AMP-LIST"
requires_extension: "amp-list"
attrs: {
name: "binding"
value: "always|refresh|no"
Copy link
Contributor

Choose a reason for hiding this comment

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

value is now a repeated field, so this can listed as

  value: "always"
  value: "no"
  value: "refresh"

Same below

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@codecov-io
Copy link

codecov-io commented Jul 18, 2018

Codecov Report

Merging #16833 into master will decrease coverage by 0.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master   #16833      +/-   ##
==========================================
- Coverage   78.13%   78.12%   -0.02%     
==========================================
  Files         560      560              
  Lines       40641    40645       +4     
==========================================
- Hits        31756    31752       -4     
- Misses       8885     8893       +8
Flag Coverage Δ
#integration_tests 34.54% <ø> (+0.01%) ⬆️
#unit_tests 77.15% <91.66%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f06595...a1052af. Read the comment docs.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

validation changes look good.

@dreamofabear
Copy link
Author

/to @aghassemi @cathyxz Ali and I briefly discussed this awhile back but let me know if you'd like me to bring it to design review.

const binding = this.element.getAttribute('binding');
// "no": Always skip binding update.
if (binding === 'no' ||
isExperimentOn(this.win, 'amp-list-binding-no')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

replace disable-faster-amp-list in experiments.js with this?

Copy link
Author

Choose a reason for hiding this comment

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

Removed that defunct experiment.

These experiments are mostly for private testing, but I think I can just add the binding attribute via devtools instead. Removed these too.

// "refresh": Do _not_ block on retrieval of the Bind service before the
// first mutation (AMP.setState).
if (binding === 'refresh' ||
isExperimentOn(this.win, 'amp-list-binding-refresh')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the ideal of these experiments to allow doc-level/origin trial optin?

Copy link
Author

Choose a reason for hiding this comment

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

No, I wanted to test if some existing prod pages would benefit from this. Removed per comment above.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

.md changes when in prod?

@dreamofabear
Copy link
Author

.md changes when in prod?

👍

@dreamofabear dreamofabear merged commit c3e8d15 into ampproject:master Jul 20, 2018
@dreamofabear dreamofabear deleted the amp-list-binding branch July 20, 2018 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants