Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

fix(list): maintain classes with state.listItemClassNames #776

Merged
merged 8 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
265 changes: 157 additions & 108 deletions package-lock.json

Large diffs are not rendered by default.

15 changes: 10 additions & 5 deletions packages/list/ListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@ export interface ListItemProps<T> extends React.HTMLProps<T> {
tag?: string;
activated?: boolean;
selected?: boolean;
};
onDestroy?: () => void;
}

// TODO: convert to functional component
// https://github.com/material-components/material-components-web-react/issues/729
export default class ListItem<T extends HTMLElement = HTMLElement> extends React.Component<
ListItemProps<T>,
{}
Expand All @@ -53,14 +52,20 @@ export default class ListItem<T extends HTMLElement = HTMLElement> extends React
onClick: () => {},
onFocus: () => {},
onBlur: () => {},
onDestroy: () => {},
tag: 'li',
};

componentWillUnmount() {
this.props.onDestroy!();
}

get classes() {
const {className, activated, selected} = this.props;
const {className, activated, disabled, selected} = this.props;
return classnames('mdc-list-item', className, {
[MDCListFoundation.cssClasses.LIST_ITEM_ACTIVATED_CLASS]: activated,
[MDCListFoundation.cssClasses.LIST_ITEM_SELECTED_CLASS]: selected,
'mdc-list-item--disabled': disabled,
});
}

Expand All @@ -84,9 +89,9 @@ export default class ListItem<T extends HTMLElement = HTMLElement> extends React
role,
checkboxList,
radioList,
onDestroy,
/* eslint-enable no-unused-vars */
tag: Tag,

...otherProps
} = this.props;
return (
Expand Down
4 changes: 1 addition & 3 deletions packages/list/ListItemGraphic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import * as React from 'react';
import classnames from 'classnames';

export interface ListItemGraphicProps {
tabbableOnListItemFocus?: boolean;
className?: string;
tabIndex?: number;
graphic: React.ReactElement<any>;
Expand All @@ -34,13 +33,12 @@ export interface ListItemGraphicProps {
const ListItemGraphic:React.FunctionComponent<ListItemGraphicProps> = ({
tabIndex = -1, // eslint-disable-line no-unused-vars
graphic,
tabbableOnListItemFocus = false,
className = '',
...otherProps
}) => {
const graphicProps = {
className: classnames('mdc-list-item__graphic', className),
tabIndex: tabbableOnListItemFocus ? tabIndex : -1,
tabIndex: tabIndex !== undefined ? tabIndex : -1,
...otherProps,
};
return React.cloneElement(graphic, graphicProps);
Expand Down
6 changes: 2 additions & 4 deletions packages/list/ListItemMeta.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,16 @@ import * as React from 'react';
import classnames from 'classnames';

export interface ListItemMetaProps {
tabbableOnListItemFocus?: boolean;
className?: string;
tabIndex?: number;
meta: string | React.ReactElement<any>;
childrenTabIndex?: number;
};

const ListItemMeta:React.FunctionComponent<ListItemMetaProps> = ({
tabIndex = -1, // eslint-disable-line no-unused-vars
tabIndex, // eslint-disable-line no-unused-vars
meta,
className = '',
tabbableOnListItemFocus = false,
...otherProps
}) => {
let metaElement: React.ReactElement<any>;
Expand All @@ -46,7 +44,7 @@ const ListItemMeta:React.FunctionComponent<ListItemMetaProps> = ({
}
const metaProps = {
className: classnames('mdc-list-item__meta', className, metaElement.props.className),
tabIndex: tabbableOnListItemFocus ? tabIndex : -1,
tabIndex: tabIndex !== undefined ? tabIndex : -1,
...otherProps,
};
return React.cloneElement(metaElement, metaProps);
Expand Down
6 changes: 2 additions & 4 deletions packages/list/ListItemText.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import * as React from 'react';
import classnames from 'classnames';

export interface ListItemTextProps {
tabbableOnListItemFocus?: boolean;
tabIndex?: number;
className?: string;
primaryText?: React.ReactNode;
Expand All @@ -41,7 +40,6 @@ const ListItemText: React.FunctionComponent<ListItemTextProps> = ({
/* eslint-disable react/prop-types */
primaryText = '',
secondaryText = '',
tabbableOnListItemFocus = false,
tabIndex = -1,
className = '',
/* eslint-enable react/prop-types */
Expand All @@ -53,7 +51,7 @@ const ListItemText: React.FunctionComponent<ListItemTextProps> = ({
return (
<span
className={className}
tabIndex={tabbableOnListItemFocus ? tabIndex : -1}
tabIndex={tabIndex !== undefined ? tabIndex : -1}
>
{text}
</span>
Expand All @@ -78,7 +76,7 @@ const ListItemText: React.FunctionComponent<ListItemTextProps> = ({
return (
<span
className={classnames('mdc-list-item__text', className)}
tabIndex={tabbableOnListItemFocus ? tabIndex : -1}
tabIndex={tabIndex !== undefined ? tabIndex : -1}
{...otherProps}
>
{renderText(primaryText, 'mdc-list-item__primary-text')}
Expand Down
3 changes: 0 additions & 3 deletions packages/list/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,6 @@ class MyApp extends Component {
| ----------------------- | ------- | ----------------------------------------------------------------------------------------------------------------- |
| className | String | Classes to be applied to the list item text element |
| tabIndex | Number | Tab index of the list item text |
| tabbableOnListItemFocus | Boolean | Whether focusing list item will toggle tab index of the list item text. If false, the tab index will always be -1 |
| primaryText | String | Primary text for the list item |
| secondaryText | String | Secondary text for the list item |

Expand All @@ -322,7 +321,6 @@ class MyApp extends Component {
| ----------------------- | ------- | -------------------------------------------------------------------------------------------------------------------- |
| className | String | Classes to be applied to the list item graphic element |
| tabIndex | Number | Tab index of the list item graphic |
| tabbableOnListItemFocus | Boolean | Whether focusing list item will toggle tab index of the list item graphic. If false, the tab index will always be -1 |
| graphic | Element | The graphic element to be displayed in front of list item text |

### ListItemMeta
Expand All @@ -331,7 +329,6 @@ class MyApp extends Component {
| ----------------------- | ----------------- | ----------------------------------------------------------------------------------------------------------------- |
| className | String | Classes to be applied to the list item meta element |
| tabIndex | Number | Tab index of the list item meta |
| tabbableOnListItemFocus | Boolean | Whether focusing list item will toggle tab index of the list item meta. If false, the tab index will always be -1 |
| meta | Element or String | The meta element or string to be displayed behind list item text |

### ListDivider
Expand Down
121 changes: 90 additions & 31 deletions packages/list/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ export interface ListProps<T extends HTMLElement> extends React.HTMLProps<HTMLEl
children: ListItem<T> | ListItem<T>[] | React.ReactNode;
};


interface ListState {
listItemClassNames: {[listItemIndex: number]: string[]},
}

function isReactText(element: any): element is React.ReactText {
return typeof element === 'string' || typeof element === 'number';
}
Expand All @@ -63,13 +68,18 @@ function isSelectedIndexType(selectedIndex: unknown): selectedIndex is MDCListIn
return typeof selectedIndex === 'number' && !isNaN(selectedIndex) || Array.isArray(selectedIndex);
}

export default class List<T extends HTMLElement = HTMLElement> extends React.Component<ListProps<T>, {}> {
export default class List<T extends HTMLElement = HTMLElement> extends React.Component<ListProps<T>, ListState> {
listItemCount = 0;
foundation!: MDCListFoundation;
hasInitializedListItem = false;
hasInitializedListItemTabIndex = false;
hasInitializedList = false;

private listElement = React.createRef<HTMLElement>();

state: ListState = {
listItemClassNames: {},
};

static defaultProps: Partial<ListProps<HTMLElement>> = {
'className': '',
'checkboxList': false,
Expand Down Expand Up @@ -100,6 +110,11 @@ export default class List<T extends HTMLElement = HTMLElement> extends React.Com
this.props[ARIA_ORIENTATION] === VERTICAL
);
this.initializeListType();

// tabIndex for the list items can only be initialized after
// the above logic has executed. Once this is true, we need to call forceUpdate.
this.hasInitializedList = true;
this.forceUpdate();
}

componentDidUpdate(prevProps: ListProps<T>) {
Expand Down Expand Up @@ -185,16 +200,30 @@ export default class List<T extends HTMLElement = HTMLElement> extends React.Com
listItem.setAttribute(attr, value);
}
},
/**
* Pushes class name to state.listItemClassNames[listItemIndex] if it doesn't yet exist.
*/
addClassForElementIndex: (index, className) => {
const listItem = this.listElements[index];
if (listItem) {
listItem.classList.add(className);
const {listItemClassNames} = this.state;
if (listItemClassNames[index] && listItemClassNames[index].indexOf(className) === -1) {
listItemClassNames[index].push(className);
} else {
listItemClassNames[index] = [className];
}
this.setState({listItemClassNames});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test this with disabled list item?

React.Children.map runs on each list item but these adapters excludes disabled list item when indexing.

Copy link
Author

Choose a reason for hiding this comment

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

good catch - updated and added tests

},
/**
* Finds the className within state.listItemClassNames[listItemIndex], and removes it
* from the array.
*/
removeClassForElementIndex: (index, className) => {
const listItem = this.listElements[index];
if (listItem) {
listItem.classList.remove(className);
const {listItemClassNames} = this.state;
if (listItemClassNames[index]) {
const removalIndex = listItemClassNames[index].indexOf(className);
if (removalIndex !== -1) {
listItemClassNames[index].splice(removalIndex, 1);
this.setState({listItemClassNames});
}
}
},
setTabIndexForListItemChildren: (listItemIndex, tabIndexValue) => {
Expand Down Expand Up @@ -251,6 +280,44 @@ export default class List<T extends HTMLElement = HTMLElement> extends React.Com
return null;
}

/**
* Initializes the tabIndex prop for the listItems. tabIndex is determined by:
* 1. if selectedIndex is an array, and the index === selectedIndex[0]
* 2. if selectedIndex is a number, and the the index === selectedIndex
* 3. if there is no selectedIndex
*/
private getListItemInitialTabIndex = (index: number) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To simplify this, I think List should set tabindex to first ListItem if none of the items are selected (No matter the variant). tabindex should be set to selected index otherwise. (First selected index item if it Array).

Copy link
Author

Choose a reason for hiding this comment

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

@abhiomkar you're saying it doesn't matter if its checkbox or radio type list? It should only be set to the first selectedIndex?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Hmm...ya that sounds a lot simpler than what i have...I will update.

Copy link
Author

Choose a reason for hiding this comment

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

ya that worked like a charm. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing return type. And in other places.

Copy link
Author

Choose a reason for hiding this comment

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

I think it is better for the TS compiler to infer the type here. Otherwise the return type is {}

const {selectedIndex} = this.props;
let tabIndex = {};
if (this.hasInitializedList && !this.hasInitializedListItemTabIndex) {
const isSelectedIndexArray
= Array.isArray(selectedIndex) && selectedIndex.length > 0 && index === selectedIndex[0];
const isSelected = selectedIndex === index;
if (isSelectedIndexArray || isSelected || selectedIndex === -1) {
tabIndex = {tabIndex: 0};
this.hasInitializedListItemTabIndex = true;
}
}

return tabIndex;
}

/**
* Method checks if the list item at `index` contains classes. If true,
* method merges state.listItemClassNames[index] with listItem.props.className.
* The return value is used as the listItem's className.
*/
private getListItemClassNames = (index: number, listItem: React.ReactElement<ListItemProps<T>>) => {
let {className = ''} = listItem.props;
const {listItemClassNames} = this.state;
if (listItemClassNames[index]) {
listItemClassNames[index];
className = classnames(className, listItemClassNames[index]);
}

return className;
}

handleKeyDown = (e: React.KeyboardEvent<any>, index: number) => {
e.persist(); // Persist the synthetic event to access its `key`
this.foundation.handleKeydown(
Expand Down Expand Up @@ -317,46 +384,33 @@ export default class List<T extends HTMLElement = HTMLElement> extends React.Com

renderChild = (child: React.ReactElement<ListItemProps<T>> | React.ReactChild) => {
if (!isReactText(child) && isListItem(child)) {
return this.renderListItem(child, this.listItemCount++);
return this.renderListItem(child, child.props.disabled ? -1 : this.listItemCount++);
}
return child;
};

renderListItem = (listItem: React.ReactElement<ListItemProps<T>>, index: number) => {
const {checkboxList, radioList, selectedIndex} = this.props;
let tabIndex = {};
const {checkboxList, radioList} = this.props;
const tabIndex = this.getListItemInitialTabIndex(index);
const className = this.getListItemClassNames(index, listItem);

const {
onKeyDown,
onClick,
onFocus,
onBlur,
/* eslint-disable no-unused-vars */
className: _classNames,
/* eslint-enable no-unused-vars */
...otherProps
} = listItem.props;

if (!this.hasInitializedListItem) {
const isSelectedIndexArray = Array.isArray(selectedIndex) && selectedIndex.length > 0;
// if selectedIndex is populated then check if its a checkbox/radioList
if (selectedIndex && (isSelectedIndexArray || selectedIndex > -1)) {
const isCheckboxListSelected
= checkboxList && Array.isArray(selectedIndex) && selectedIndex.indexOf(index) > -1;
const isNonCheckboxListSelected = selectedIndex === index;
if (isCheckboxListSelected || isNonCheckboxListSelected) {
tabIndex = {tabIndex: 0};
this.hasInitializedListItem = true;
}
// set tabIndex=0 to first listItem if selectedIndex is not populated
} else {
tabIndex = {tabIndex: 0};
this.hasInitializedListItem = true;
}
}


const props = {
// otherProps must come first
// otherProps must be first
...otherProps,
checkboxList,
radioList,
className,
onKeyDown: (e: React.KeyboardEvent<T>) => {
onKeyDown!(e);
this.handleKeyDown(e, index);
Expand All @@ -373,6 +427,11 @@ export default class List<T extends HTMLElement = HTMLElement> extends React.Com
onBlur!(e);
this.handleBlur(e, index);
},
onDestroy: () => {
const {listItemClassNames} = this.state;
delete listItemClassNames[index];
this.setState({listItemClassNames});
},
...tabIndex,
};
return React.cloneElement(listItem, props);
Expand Down
2 changes: 1 addition & 1 deletion test/screenshot/golden.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"layout-grid": "fe40f7a34853bc2a1d9a836e812599d4d47b5b26b839a8eaed7f98ea91946790",
"line-ripple": "56b136db2dc7e09260849447e6bde9b55a837af332a05d9f52506ab1c95e2e57",
"linear-progress": "f7a23058842b37875a02b3562e0b9c82f7fb24f9b88e9727d10b921b001d115e",
"list": "4dcdfeb03d781eca5367f6cbf4c7ebd13d1fe8932ca5758520b994d541f49095",
"list": "e633e9536eeadfe7bbb46cdf3d42c5ebf40a8974420f6183057b6f7ddcbeca21",
"material-icon": "442b39fb22d2c7a74efb23ca098429b471501ce21df8662327bbf9871fe0bcb0",
"menu-surface": "f5face1a24fe166e86e8a3dc35ea85b2d4431469a3d06bf6fc1a30fbdc175aff",
"notched-outline": "7770dd381c27608a1f43b6f83da92507fe53963f5e4409bd73184b86275538fe",
Expand Down
Loading