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

[Lists] Change List and ListItem to use ul and li #1168

Closed
bweggersen opened this issue Jul 15, 2015 · 17 comments
Closed

[Lists] Change List and ListItem to use ul and li #1168

bweggersen opened this issue Jul 15, 2015 · 17 comments
Labels
accessibility a11y component: list This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@bweggersen
Copy link

We are looking through the semantics of our website, which uses Material-UI, and we discovered that list items in Material UI doesn't use <ul> and <li> (ie https://github.com/callemall/material-ui/blob/master/src/lists/list-item.jsx#L298). Does anybody know why this is?

@hai-cea
Copy link
Member

hai-cea commented Jul 16, 2015

@bweggersen That's a good question :)

We should probably change it to use ul and li.

@hai-cea hai-cea changed the title Why don't we use <li> in lists? [Lists] Why don't we use <li> in lists? Jul 16, 2015
@hai-cea hai-cea changed the title [Lists] Why don't we use <li> in lists? [Lists] Change List and ListItem to use ul and li Jul 16, 2015
@cgestes
Copy link
Contributor

cgestes commented Jul 16, 2015

+1 let's keep the semantic of webpages structure correct.

@shaurya947
Copy link
Contributor

@bweggersen @cgestes are either of you interested in taking this up in a PR?

@alitaheri alitaheri added new feature New feature or request and removed Enhancement labels Dec 8, 2015
@alitaheri
Copy link
Member

@oliviertassinari @newoga @mbrookes Let's discuss this too. I think using ul and li might limit what we can do and hurt composibility, I'm not sure. what do you guys think?

@newoga
Copy link
Contributor

newoga commented Jan 28, 2016

@alitaheri I think this PR is suggesting to change the underlying DOM node to render ul and li instead of div. I don't know how this would hurt composability and I think is more semantic so I'm okay with this change. Anything I'm missing?

@alitaheri
Copy link
Member

well my only concern is that, ul can only contain li. if that doesn't limit us, then I'm ok with this too.

@newoga
Copy link
Contributor

newoga commented Jan 28, 2016

Yeah that's fair. I think we'd just have to implement it in a way where ListItems and custom composed components are just wrapped in an <li> that don't have any styling. I suppose the only caveat is that browsers more often then not have predefined styling for those tags that we may have to reset to avoid weird rendering issues. But I think it's still worth looking at.

I think more semantic tags definitely help in terms of finding stuff in the DOM, especially with the inline style approach.

@alitaheri
Copy link
Member

Yeah, keeping the semantic is very good. but limiting somehow. We'll have to investigate this before trying to implement it.

@omarreiss
Copy link

I just wanted to add that semantic HTML is also very important for accessibility. Not having this for example makes things less accessible for people using screen readers. cc: @afercia

@oliviertassinari oliviertassinari added the component: list This is the name of the generic UI component, not the React module! label Dec 18, 2016
@mbrookes mbrookes added Enhancement and removed new feature New feature or request labels Jan 17, 2017
@mbrookes
Copy link
Member

@nathanmarks I see next is still using div by default. Is using ul / li a possibility, or should I mark as wontfix?

@oliviertassinari oliviertassinari added v1-alpha good first issue Great for first contributions. Enable to learn the contribution process. accessibility a11y labels Jun 29, 2017
@oliviertassinari oliviertassinari added this to the v1.0.0-prerelease milestone Jul 4, 2017
@akshaynaik404
Copy link
Contributor

Is someone already working on this? If not, How should I go about solving it?

@mbrookes
Copy link
Member

@akshaynaik404 No-one is working on this. If you wanted to help, the contributing guide has some pointers on how to get started.

oliviertassinari pushed a commit that referenced this issue Jul 18, 2017
In List.js change default prop 'component' to 'ul' and make corresponding
changes to List.spec.js and in ListItem.js change default prop 'component'
to 'li'.

Now by default List should be a 'ul' and ListItem should be an 'li'.

Also changed affected integration test in MenuList.spec.js.
@pssuralkar
Copy link

HI,
I am trying to warp with div but getting error "Warning: Unknown prop nestedLevel on

tag. Remove this prop from the element." so could you please help me how to resolve this issue or is there any alternative solution to fix this issue. Thanks in Advance.

@pssuralkar
Copy link

I am trying to warp "ListItem " with div

@akshaynaik404
Copy link
Contributor

@oliviertassinari will be in a better position to answer your query. It would also be helpful if you shared a code snippet along as well.

@mbrookes
Copy link
Member

@pssuralkar @akshaynaik404, best to use github issues for... issues. Please keep deployment questions on gitter or SO.

In general when wrapping a component, the wrapper needs to pass through to the child component any props that the parent component expects the child to support.

@pssuralkar
Copy link

pssuralkar commented Oct 18, 2017

@akshaynaik404 - Below is the code. I want to wrapped list item in div for some CSS styling.

 <div className={"verticalLine"}>
                <ListItem
                  key={1}
                  className={this.props.route.route == "/" ? "active" : "null"}
                  primaryText={"List Item"}
                  secondaryText={<span className={"horizontalLine"}>{}</span>}
                  leftIcon={<img src="icon.svg"/>}
                  primaryTogglesNestedList={true}
                  initiallyOpen={true}
                />
</div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: list This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests