Skip to content

amp-list: customize array location in the response#6745

Merged
dvoytenko merged 4 commits intoampproject:masterfrom
dvoytenko:amp-list-root1
Dec 20, 2016
Merged

amp-list: customize array location in the response#6745
dvoytenko merged 4 commits intoampproject:masterfrom
dvoytenko:amp-list-root1

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

Fixes #6507.

/cc @dandv, @ericlindley-g for the feature
/cc @Gregable @honeybadgerdontcare @powdercloud for validator changes
/cc @bpaduch for spec/docs changes

@Gregable
Copy link
Copy Markdown
Member

looks good for validator changes. ping me when you've submitted it.

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

One comment.

if (expr == '.') {
return obj;
}
// Otherwise, navigate via properties.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Switch hasOwnProperty to Object.prototype.hasOwnProperty.call

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Dima Voytenko added 2 commits December 19, 2016 19:06
@ericlindley-g
Copy link
Copy Markdown
Contributor

Nice—excited to get this enhancement out!

@dvoytenko dvoytenko merged commit c70061f into ampproject:master Dec 20, 2016
@dvoytenko dvoytenko deleted the amp-list-root1 branch December 20, 2016 19:46
@dandv
Copy link
Copy Markdown
Contributor

dandv commented Dec 22, 2016

Thanks! When should we see this in production?

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* amp-list: customize array location in the response

* lints

* review fixes

* lints
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* amp-list: customize array location in the response

* lints

* review fixes

* lints
@dvoytenko
Copy link
Copy Markdown
Contributor Author

After Jan 5th. Please see https://github.com/ampproject/amphtml/releases

torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jan 3, 2017
* amp-list: customize array location in the response

* lints

* review fixes

* lints
@jalajc
Copy link
Copy Markdown

jalajc commented Jan 6, 2017

Oh, so still slated for release...
As the documentation for this change is already updated here, I was trying... to no avail and getting error Response must be {items: []} object. Would appretiate is documentation includes a nested navigation example (or may be at https://www.ampbyexample.com).

(feature request is somewhat referenced here too: #3554 )

@dvoytenko
Copy link
Copy Markdown
Contributor Author

@jalajc This feature should be already on PROD (via #6745). Do you have an example you tried with? It should work, so if not - please send it to me and I'll debug.

@jalajc
Copy link
Copy Markdown

jalajc commented Jan 10, 2017

@dvoytenko, Yups it was in prod the next day I wrote here. Nested list is perfect now.

I didn't update here as I was facing some issue with cliping of content (in terms of visibility on ui) which was not happening earlier and I was trying to figure out if my config/html layout is faulty.

@jalajc
Copy link
Copy Markdown

jalajc commented Mar 6, 2017

One thing I notice is for nested list item, you still have to specify the src mandatorily and even src url is same that url endpoint is called as many times as you specify it.

To elaborate consider the following snippet for which mustache template is already defined as relatedLy

<amp-list width=300 height=75 layout=responsive template="relatedLy" src='https://suraurgeet_com/json/testrel.json' items="items1"></amp-list>
<amp-list width=300 height=75 layout=responsive template="relatedLy" src='https://suraurgeet_com/json/testrel.json' items="items2"></amp-list>
<amp-list width=300 height=75 layout=responsive template="relatedLy" src='https://suraurgeet_com/json/testrel.json' items="items3.items4"></amp-list>

Notice that

  1. src attribute is same in all three and its response contains objects items1/2/3/4
  2. items attribute is different in all three

Now whats happening is src, the CORS endpoint, is called three times!!

Can some configuration be done to avoid unnecessary repeated call to CORS end point.
Can src be not made optional, in that case use url specified in template and issue a warning in console in dev mode; but call that end point only once in a session as its not a live-list.

(Its like I am billed 3x for api that actually owe 1x 😃, plus more processing and latency )

@dvoytenko
Copy link
Copy Markdown
Contributor Author

@cramforce Would this be solved by the datasource idea that you had?

@jridgewell
Copy link
Copy Markdown
Contributor

This will be handled by #7562.

@cramforce
Copy link
Copy Markdown
Member

Yes #7562 will do this.

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.

7 participants