ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

issues with amp-list in amp-selector #9536

Closed aghassemi closed 6 years ago

aghassemi commented 7 years ago

if amp-list renders new options inside an amp-selector, amp-selector has no idea and does not rescan children to find these new options.

Repro case from @gustavopaes

<!-- Template -->
<amp-selector name="fipe-category" layout="container" on="select:AMP.setState({controller: {category: event.targetOption}})">
    <ul>
        <amp-list
            src="https://carros.uol.com.br/service.htm?type=areas/carros/fipe-table-api&args={'params':{'category':1}}&json"
            height="100"
            width="100"
            [src]="controller.category == false ? '' : config.service"
            items="docs"
            layout="responsive"
            >
            <template type="amp-mustache">
                <li option="{{.}}" value="{{.}}">{{.}}</li>
            </template>
        </amp-list>
    </ul>
</amp-selector>

<!-- After javascript run -->
<amp-selector name="fipe-category" layout="container" on="select:AMP.setState({controller: {category: event.targetOption}})" class="i-amphtml-element i-amphtml-layout-container i-amphtml-layout" role="listbox">
    <ul>
        <amp-list src="https://carros.uol.com.br/tabela-fipe/index.amp.htm" height="100" width="100" [src]="controller.category == false ? '' : config.service" items="docs" layout="responsive" class="i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-layout" aria-live="polite" style="height: 2088px;">
            <template type="amp-mustache">
                <li option="{{.}}" value="{{.}}">{{.}}</li>
            </template>

            <div class="i-amphtml-fill-content i-amphtml-replaced-content" role="list">
                <!-- Look the option, it's empty and it's required by amp-selector -->
                <li option="" value="Acura" role="listitem">Acura</li>
                <li option="" value="Agrale" role="listitem">Agrale</li>
                <li option="" value="Alfa Romeo" role="listitem" aria-selected="false">Alfa Romeo</li>
                <li option="" value="Walk" role="listitem">Walk</li>
            </div>
        </amp-list>
    </ul>
    <input type="hidden" name="fipe-category" value="">
</amp-selector>

/to @camelburrito /cc @gustavopaes

cvializ commented 7 years ago

If @gustavopaes hasn't already found a workaround, the following example can work. If you can change the endpoint, you can wrap the data for amp-list in a single-item array and render all of amp-selector inside the amp-list template e.g.

{"items": [{
    "docs": [/*...*/]
]}
<amp-list
    src="https://carros.uol.com.br/service.htm?type=areas/carros/fipe-table-api&args={'params':{'category':1}}&json"
    height="100"
    width="100"
    [src]="controller.category == false ? '' : config.service"
    layout="responsive"
    >
    <template type="amp-mustache">
        <amp-selector
            name="fipe-category"
            layout="container"
            on="select:AMP.setState({controller: {category: event.targetOption}})"
            >
            <ul>
                {{#docs}}<li option="{{.}}" value="{{.}}">{{.}}</li>{{/docs}}
            </ul>
        </amp-selector>
    </template>
</amp-list>
kmh287 commented 7 years ago

Would that put an amp-selector into each rendered item?

cvializ commented 7 years ago

amp-selector would only render once. There is only one rendered "item" since the items array only has one element. It wraps the object containing docs so that the whole template isn't repeated multiple times. I used a similar strategy for my autocomplete demo.

kmh287 commented 7 years ago

Ah, interesting workaround!

cvializ commented 7 years ago

Oh! I forgot, the option attribute needs to be whitelisted in templates before this will work anyway. That is part of PR #9553

aghassemi commented 7 years ago

cool workaround!

ampprojectbot commented 7 years ago

This issue hasn't been updated in awhile. @camelburrito Do you have any updates?

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @camelburrito Do you have any updates?

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

kevinkassimo commented 6 years ago

Taking this issue