Skip to content
This repository has been archived by the owner on Oct 22, 2019. It is now read-only.

amp-list markup needs improvment. #441

Closed
aghassemi opened this issue Oct 21, 2016 · 3 comments
Closed

amp-list markup needs improvment. #441

aghassemi opened this issue Oct 21, 2016 · 3 comments

Comments

@aghassemi
Copy link
Contributor

https://ampbyexample.com/components/amp-list/ has invalid HTML setup and there is no need for the wrapper UL and LI children.

They should just be:

    <amp-list width=auto
        height=100
        layout=fixed-height
        src="https://ampbyexample.com/json/examples.json">
      <template type="amp-mustache"
          id="amp-template-id">
        <div>
          <a href={{url}}>{{title}}</a>
        </div>
      </template>
    </amp-list>

We should add some text to the example that ensures the author we are using proper aria roles to make these divs list items

If we really like to allow folks to use 'li' as children, then amp-list needs to become smarter and not create the container until it has a child and can inspect the child to see if it is a li or not and based on that either create a div or an ul

See ampproject/amphtml#5513 (comment)

@lannka @kul3r4

@sebastianbenz
Copy link
Collaborator

amp-list is definitely the weirdest and most confusing AMP component we have. I find it very misleading to name a component amp-list, but then not support html list tags (and ironically having to add the corresponding list aria attributes afterwards). Not talking about how hard it is too layout an amp-list...

Sorry for the rant. Thanks for lettings us know! Will fix it.

sebastianbenz added a commit to sebastianbenz/amp-by-example that referenced this issue Oct 21, 2016
@sebastianbenz
Copy link
Collaborator

Having said that - I now realise my mistake. I was always thinking of amp-list as a component for generating lists. Which is wrong: amp-list is a list. Once I see it that way, everything makes sense (also why the table tag is not supported inside amp-list). Seems like I'm not yet done groking web components.

@kul3r4 kul3r4 closed this as completed in 2947686 Oct 21, 2016
@aghassemi
Copy link
Contributor Author

@sebastianbenz yeah IMO the confusing part comes from not knowing whether amp-list is simply a loop control structure and not an actual DOM element (in which case the existing code totally makes sense) or it being an actual list itself (which is the case). At least in AMP custom elements, so far, we have mostly avoided elements that are purely control elements. That may not last much longer though :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants