feat(list): allow semantic tags (mainly, nav on List and a on ListItem) #405

4cm4k1 · 2018-11-02T06:16:57Z

Backstory

This PR began as an attempt to help with the focus-trap issues that using Drawer and List together were causing (reported here: #382; tracked here: #433).

Purpose

What this PR now is simply adding the ability to override the default tags (nav [default: ul] on List and a [default: li] on ListItem) by using the tag prop.

Context

Changing the tags would be recommended when using these components to render a page's navigation, as it is semantic HTML and helps with a11y. Additionally, this may help with compatibility with routing libraries such as React Router and Next.js' Link.

Notes

I've also added two unit tests corresponding to the functionality added by this PR and added text regarding this feature in List's README.md.

…esence of href prop

…efault

…href is present

… <nav> tag, fix typos

codecov-io · 2018-11-02T06:20:43Z

Codecov Report

Merging #405 into rc7.0 will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            rc7.0     #405   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files          55       55           
  Lines        1849     1849           
  Branches      217      217           
=======================================
  Hits         1778     1778           
  Misses         71       71

Impacted Files	Coverage Δ
packages/list/index.js	`89.09% <ø> (ø)`	⬆️
packages/list/ListItem.js	`87.5% <ø> (ø)`	⬆️

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 3a8447a...a063d01. Read the comment docs.

moog16

@4cm4k1 thanks for opening the PR...however I don't think it really fixes the issue. There actually needs to be a tabIndex attribute on the list item in order to suppress the error from the component.

Regardless I agree that these changes should go in, even though it doesn't address the issue. Could you please also add the tag attrs to the README as well?

moog16 · 2018-11-02T18:35:33Z

packages/list/ListItem.js

@@ -72,14 +72,16 @@ export default class ListItem extends Component {
      ...otherProps
    } = this.props;

+    const SemanticListItem = this.props.href ? 'a' : 'li';


I do agree with this change, but I think there should be a way to programatically change routes. Things like React Router and SPA architecture require this. Semantically we will want an anchor tag, but no href, correct?

I think so? I'm having a hard time with the layers of abstraction here. Below is an example of how I would imagine it working while using next and its routing logic via next/link.

import Link from 'next/link'; import List, { ListItem, ListItemGraphic, ListItemText, } from '@material/react-list'; const NavList = [ { href: '/', name: 'Home', icon: 'home', }, { href: '/info', name: 'Info', icon: 'info', } ]; export default () => ( <List wrapFocus tag="nav"> {NavList.map(({ href, name, icon }, index) => ( <Link key={index} href={href} prefetch passHref> <ListItem key={index} tag="a"> <ListItemGraphic graphic={<MaterialIcon icon={icon} />} /> <ListItemText primaryText={name} /> </ListItem> </Link> // With react-router-dom it would be pretty similar... // <Link to={href}> // <ListItem ...> // <ListItemGraphic ... /> // <ListItemText ... /> // </ListItem> // </Link> ))} </List> );

What if people wanted to do

<ListItem key={index} tag="a" onClick={this.goToAnotherRoute}> <ListItemGraphic graphic={<MaterialIcon icon={icon} />} /> <ListItemText primaryText={name} /> </ListItem>

All I'm saying is, we should remove const SemanticListItem = this.props.href ? 'a' : 'li';, in favor of the Tag property.

What if people wanted to do

<ListItem key={index} tag="a" onClick={this.goToAnotherRoute}> <ListItemGraphic graphic={<MaterialIcon icon={icon} />} /> <ListItemText primaryText={name} /> </ListItem>

All I'm saying is, we should remove const SemanticListItem = this.props.href ? 'a' : 'li';, in favor of the Tag property.

Got it. I've updated ListItem accordingly and updated the README.

Another thing to consider: When using router frameworks that nest elements within <Link></Link>, I encountered problems where the error output is something to the effect of Cannot set property 'aria-selected' of undefined, and when using the selectedIndex prop, it’s unable to set it on the index specified. Would this be fixed if users extended the Link component to pass the necessary props to its children (i.e., ListItem)? Or how else can we make it safe to nest ListItem?

I have the beginnings of that in my project, though currently using RMWC because of the crashing issues here, shown here:

<Link {...props}> <ListItem tag="a" activated={activated}> {children} </ListItem> </Link>

4cm4k1 · 2018-11-02T20:15:52Z

@moog16 Thank you for pointing that out. ListItemText, ListItemGraphic, and ListItemMeta have tabIndex attributes currently, while ListItem does not. What would be the best way, then, to implement it for ListItem? I guess I'm a bit confused as to why the children of ListItem have tabIndex but not ListItem.

I updated the readme with the tag prop name addition to List.

Update: By default, it seems tagindex="0" is set on the first ListItem, with tagindex="-1" being set on the remaining number of ListItem, and that doesn't suppress the error. Rather, an <a> tag does.

* master: fix(list): selectedIndex 0 isn't working (material-components#387)

moog16 · 2018-11-06T19:11:01Z

@4cm4k1 tabIndex is rather complicated, but necessary for a11y. WCAG states we should be able to tab to each separate , but arrow through each . We also must add tabindex=-1 to all child elements, since some of them might have tabIndex.

I haven't had time this week to check, but you're saying using with tabIndex does not solve modal issue of crashing?

4cm4k1 · 2018-11-06T20:38:47Z

@4cm4k1 tabIndex is rather complicated, but necessary for a11y. WCAG states we should be able to tab to each separate , but arrow through each . We also must add tabindex=-1 to all child elements, since some of them might have tabIndex.

I haven't had time this week to check, but you're saying using with tabIndex does not solve modal issue of crashing?

Correct. The crash issue only seemed to go away after using <a> tags instead of <li>. I totally understand and support the need for a11y; I’m just admittedly not aware of the technical details.

…ial-components-web-react into fix/list/allow-alternate-tags * 'master' of https://github.com/material-components/material-components-web-react: docs(menu-surface): open state & ref errors (material-components#412) fix(text-field): allow for input isValid override (material-components#374) feat(list): Add List Group and List Group Subheader (material-components#386) fix(top-app-bar): allow react elements in title (material-components#376) docs: update main component manifest on main Readme (material-components#394) docs: update roadmap (material-components#396) docs: Add web survey link in readmes (material-components#402) docs: update main README with CRA2 guidelines (material-components#393)

…te README

moog16 · 2018-11-09T22:03:27Z

@4cm4k1 could you please fix conflicts? I'll do another review after that is done. Thanks!

moog16 · 2018-11-13T01:32:12Z

@4cm4k1 opened this issue to solve the modal crashing issue #433

…md note * master: chore: Publish fix(text-field): added reference to input element (material-components#414) fix(drawer): update readme and screenshots to support below top app bar (material-components#397) refactor(list): Refactor list item management (material-components#413)

4cm4k1 · 2018-11-13T02:29:09Z

@4cm4k1 I resolved the merge conflicts, added some text to the README.md, renamed this PR, and rewrote the PR's description to better reflect what this is. Thanks! 😄

moog16 · 2018-11-14T04:28:09Z

test/unit/list/ListItem.test.js

@@ -65,3 +65,8 @@ test('passes props.childrenTabIndex to children as props.tabIndex', () => {
  );
  assert.equal(wrapper.find('.list-item-child').props().tabIndex, 2);
 });
+


neither of the tests pass. Also could you add a test for the default cases?

Can you clarify how to get set up locally? I've been unable to successfully npm run build or npm run test:unit, which has made it difficult to validate these tests myself, and there doesn't seem to be any documentation on 'developing' the repo. For now, I've added the default cases and corrected a mistake in the list item a test. Thanks!

Tests now appear to be passing, and I also fixed some eslint errors on my part.

Sounds like you've got it figured out

Added a task to add a doc for contributions. Thanks! #440

moog16 · 2018-11-14T17:30:03Z

created a tests pr #439

moog16 · 2018-11-14T17:59:25Z

@4cm4k1 tests pass! Please sign it by commenting "I signed it" in this pr

4cm4k1 · 2018-11-14T18:01:42Z

I signed it

moog16 · 2018-11-14T18:12:05Z

Thanks @4cm4k1 !!

…m) (#405)

4cm4k1 added 4 commits November 2, 2018 00:51

fix(list): Add SemanticListItem which renders <a> or <li> based on pr…

3f31e8f

…esence of href prop

fix(list): Add optional tag prop to allow semantic <nav>, with <ul> d…

d6d0916

…efault

fix(list): Add unit test checking that list item renders with <a> if …

40f62d2

…href is present

fix(list): Add unit test checking list renders with optional semantic…

2231937

… <nav> tag, fix typos

4cm4k1 changed the title ~~fix(list): allow semantic tags (mainly <nav> and <a>), fix lack of focusable element for focus-trap~~ fix(list): allow semantic tags (mainly <nav> and <a>), fix lack of focusable element for focus-trap in <Drawer modal /> Nov 2, 2018

moog16 suggested changes Nov 2, 2018

View reviewed changes

docs(list): add tag to list prop name table

75d23b7

Merge branch 'master' into fix/list/allow-alternate-tags

bf5c4c5

* master: fix(list): selectedIndex 0 isn't working (material-components#387)

4cm4k1 added 2 commits November 6, 2018 14:39

fix(list): Add tag prop to ListItem instead of SemanticListItem, upda…

8c73ebe

…te README

4cm4k1 changed the title ~~fix(list): allow semantic tags (mainly <nav> and <a>), fix lack of focusable element for focus-trap in <Drawer modal />~~ feat(list): allow semantic tags (mainly, nav on List and a on ListItem) Nov 13, 2018

moog16 changed the base branch from master to rc7.0 November 14, 2018 04:18

Merge branch 'rc7.0' into fix/list/allow-alternate-tags

ffc0e23

moog16 reviewed Nov 14, 2018

View reviewed changes

4cm4k1 added 3 commits November 13, 2018 23:06

fix(list): Add default case unit tests, correct list item a tag test

45d0510

fix(list): Replace overlooked ul with Tag in render() of List component

64dd075

fix(list): Fix eslint errors

a063d01

moog16 mentioned this pull request Nov 14, 2018

DO NOT MERGE TESTS: 4cm4k1 fix/list/allow alternate tags #439

Closed

moog16 approved these changes Nov 14, 2018

View reviewed changes

moog16 merged commit db7daba into material-components:rc7.0 Nov 14, 2018

moog16 pushed a commit that referenced this pull request Nov 20, 2018

feat(list): allow semantic tags (mainly, nav on List and a on ListIte…

e8e6e19

…m) (#405)

moog16 pushed a commit that referenced this pull request Nov 20, 2018

feat(list): allow semantic tags (mainly, nav on List and a on ListIte…

9672cdb

…m) (#405)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(list): allow semantic tags (mainly, nav on List and a on ListItem) #405

feat(list): allow semantic tags (mainly, nav on List and a on ListItem) #405

4cm4k1 commented Nov 2, 2018 •

edited

Loading

codecov-io commented Nov 2, 2018 •

edited

Loading

moog16 left a comment

moog16 Nov 2, 2018

4cm4k1 Nov 4, 2018

moog16 Nov 5, 2018

4cm4k1 Nov 6, 2018

4cm4k1 commented Nov 2, 2018 •

edited

Loading

moog16 commented Nov 6, 2018

4cm4k1 commented Nov 6, 2018

moog16 commented Nov 9, 2018

moog16 commented Nov 13, 2018

4cm4k1 commented Nov 13, 2018

moog16 Nov 14, 2018

4cm4k1 Nov 14, 2018

4cm4k1 Nov 14, 2018

moog16 Nov 14, 2018

moog16 Nov 14, 2018

moog16 commented Nov 14, 2018

moog16 commented Nov 14, 2018

4cm4k1 commented Nov 14, 2018

moog16 commented Nov 14, 2018

feat(list): allow semantic tags (mainly, nav on List and a on ListItem) #405

feat(list): allow semantic tags (mainly, nav on List and a on ListItem) #405

Conversation

4cm4k1 commented Nov 2, 2018 • edited Loading

Backstory

Purpose

Context

Notes

codecov-io commented Nov 2, 2018 • edited Loading

Codecov Report

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4cm4k1 commented Nov 2, 2018 • edited Loading

moog16 commented Nov 6, 2018

4cm4k1 commented Nov 6, 2018

moog16 commented Nov 9, 2018

moog16 commented Nov 13, 2018

4cm4k1 commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 commented Nov 14, 2018

moog16 commented Nov 14, 2018

4cm4k1 commented Nov 14, 2018

moog16 commented Nov 14, 2018

4cm4k1 commented Nov 2, 2018 •

edited

Loading

codecov-io commented Nov 2, 2018 •

edited

Loading

4cm4k1 commented Nov 2, 2018 •

edited

Loading