[TableBody] Fix row selection re-render bug

[dchambers]

Non-selection related table re-rendering previously caused the user's
selection to be lost because the state was being reset unnecessarily.

I took the opportunity to remove the prescient TODO comment that did
correctly predict that this might be a problem.

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

[oliviertassinari]

That sounds good to me. Also, it would be good to have a unit test for that issue. Thanks.

[dchambers]

Hi @oliviertassinari, I've created a test but it doesn't actually work at present: clicking the first checkbox has no effect, whereas clicking the check-all checkbox works fine.

Is there any chance this is because the test is synchronous but relies on asynchronous behaviour, or something like that?

[oliviertassinari]

Is there any chance this is because the test is synchronous but relies on asynchronous behaviour, or something like that?

I'm not sure what's your issue it. I would rather add more tests to the MultiSelectTable.spec.js file rather than creating a new one.

[dchambers]

Hi @oliviertassinari,

The MultiSelectTable component doesn't re-render as a result of selection changes, so wouldn't replicate the bug in its present form. It could potentially be changed though, as I did in SelectionObservingTable?

However, that doesn't help the fact the test environment did not appear to be working as expected -- the checkboxes would not actually check as expected?

[oliviertassinari]

The MultiSelectTable component doesn't re-render as a result of selection changes

That could be artificialy simulated with the update API of enzyme.

However, that doesn't help the fact the test environment did not appear to be working as expected

That's most likely linked to how the test is writen.

[dennischen]

Did this issue be fixed in 0.16.7?
Regards to #1345
I have selected problem in latest 0.16.7. still, thanks

[dchambers]

Hi @dennischen, yes I can confirm it's in 0.16.7 -- I'm no longer reliant on my own fork of the code since that release came out. Maybe you're seeing a different bug?

[dennischen]

I try to build a simple single selectable list (without checkbox)
here is the code snippet(typescript)

export class Monitors extends WorkspaceComponent<MonitorsProps, MonitorsState> {

    onRowSelection = (selectedRows: number[] | string) => {
        if (selectedRows.length > 0 && 'string' !== typeof selectedRows) {
            let monitors = demoMonitors;

            this.props.onShowInfoRequest(monitors[selectedRows[0]].uuid);
        } else {
            this.props.onCloseInfoRequest();
        }
    }

    render() {
        let infoVisible = this.props.infoVisible;
        let monitors = demoMonitors;

        return <Table onRowSelection={this.onRowSelection} >
            <TableHeader displaySelectAll={false} adjustForCheckbox={false} >
                <TableRow>
                    <TableHeaderColumn>Name</TableHeaderColumn>
                    <TableHeaderColumn ></TableHeaderColumn>
                </TableRow>
            </TableHeader>
            <TableBody displayRowCheckbox={false} deselectOnClickaway={false} showRowHover={true}>
                {monitors.map((monitor: MonitorBean) => {
                    return <TableRow selected={this.props.monitorId === monitor.uuid} key={monitor.uuid}>
                        <TableRowColumn>{monitor.name}/{this.props.monitorId === monitor.uuid ? 'true' : 'false'}/ </TableRowColumn>
                        <UnSelectableTabelRowColumn style={colEnableStyle}>{infoVisible ? <span /> : <Toggle toggled={monitor.enabled} />}</UnSelectableTabelRowColumn>
                    </TableRow>
                })}
            </TableBody>
        </Table>
    }
}

the problem is when selected changes to false , the table still kept the selection. (I printed the condition in monitor name column to compare)
it is very like issue #1345

[dchambers]

it is very like issue #1345

Ah, I see! The fix I submitted was for a problem I was seeing. I wondered if it was maybe related to #1345 because that issue came up when I was searching for solutions to my problem, and it sounded similar. Sounds like I was wrong though 😊 .

Sorry for the confusion!

[namrataGogoi]

I am using the Table component of react "Material-UI v1.0.0-beta.10". In the demo, select all checkbox is working fine, But when I tried to implement it's not working. And I tried to use "multiSelectable" and "selectable" from previous version, after that too it's not working. Can you please help me fix this issue?

Below is the code:

import React from 'react';
import classNames from 'classnames';
import PropTypes from 'prop-types';
import { withStyles } from 'material-ui/styles';
import keycode from 'keycode';
import Table, {
TableBody,
TableCell,
TableHead,
TableRow
} from 'material-ui/Table';
import Toolbar from 'material-ui/Toolbar';
import Typography from 'material-ui/Typography';
import Paper from 'material-ui/Paper';
import Checkbox from 'material-ui/Checkbox';
import IconButton from 'material-ui/IconButton';
import DeleteIcon from 'material-ui-icons/Delete';
import FilterListIcon from 'material-ui-icons/FilterList';

const columnData = [
{ id: 'type', numeric: false, disablePadding: true, label: 'Type' },
{ id: 'name', numeric: true, disablePadding: false, label: 'Value' },
{ id: 'DataID', numeric: true, disablePadding: false, label: 'ID' }
];

class EnhancedTableHead extends React.Component {
constructor(props) {
super(props);
}

render() {
    const { onSelectAllClick, order, orderBy, numSelected } = this.props;

    return (
        <TableHead>
            <TableRow>
                <TableCell checkbox>
                    <Checkbox
                        indeterminate={numSelected > 0 && numSelected < 5}
                        checked={numSelected === 5}
                        onChange={onSelectAllClick}
                    />
                </TableCell>
                {columnData.map(column => {
                    return (
                        <TableCell
                            key={column.id}
                            numeric={column.numeric}
                            disablePadding={column.disablePadding}
                        >
                            {column.label}
                        </TableCell>
                    );
                }, this)}
            </TableRow>
        </TableHead>
    );
}

}

EnhancedTableHead.propTypes = {
numSelected: PropTypes.number.isRequired,
onSelectAllClick: PropTypes.func.isRequired,
order: PropTypes.string.isRequired,
orderBy: PropTypes.string.isRequired
}

const toolbarStyles = theme => ({
root: {
paddingRight: 2,
},
highlight:
theme.palette.type === 'light'
? {
color: theme.palette.secondary.A700,
backgroundColor: theme.palette.secondary.A100,
}
: {
color: theme.palette.secondary.A100,
backgroundColor: theme.palette.secondary.A700,
},
spacer: {
flex: '1 1 100%',
},
actions: {
color: theme.palette.text.secondary,
},
title: {
flex: '0 0 auto',
},
});

class EnhancedTableToolbar extends React.Component {
constructor(props) {
super(props);
}

render() {
    const { numSelected, classes } = this.props;
    return(
        <Toolbar
            className={classNames(classes.root, {
                [classes.highlight]: numSelected > 0,
            })}
        >
            <div className={classes.title}>
                {numSelected > 0 ? (
                    <Typography type="subheading">{numSelected} selected</Typography>
                ) : (
                <Typography type="title">Nutrition</Typography>
                )}
            </div>
            <div className={classes.spacer} />
            <div className={classes.actions}>
                {numSelected > 0 ? (
                    <IconButton aria-label="Delete">
                        <DeleteIcon />
                        </IconButton>
                ) : (
                <IconButton aria-label="Filter list">
                    <FilterListIcon />
                    </IconButton>
                )}
            </div>
        </Toolbar>
    )
}

};

EnhancedTableToolbar = withStyles(toolbarStyles)(EnhancedTableToolbar);

const styles = theme => ({
paper: {
width: '100%',
marginTop: theme.spacing.unit * 3,
overflowX: 'auto',
},
});

class EnhancedTable extends React.Component {
constructor(props) {
super(props);
this.state = {
order: 'asc',
orderBy: 'calories',
selected: [],
selectable: true,
multiSelectable: false,
showCheckboxes: true,
enableSelectAll: false,
data: [
{
type: 'ActivityID',
id: 1,
name: 'Rating',
DataID: 1,
},
{
type: 'Content SourceID',
id: 2,
name: 'EPG',
DataID: 1,
},
{
type: 'ChannelID',
id: 3,
name: 'Web',
DataID: 1,
},
{
type: 'ModeID',
id: 4,
name: 'Customer',
DataID: 1,
},
{
type: 'RecTypeID',
id: 5,
name: 'Preference',
DataID: 1,
}
],
};
this.handleRequestSort = this.handleRequestSort.bind(this);
this.handleSelectAllClick = this.handleSelectAllClick.bind(this);
this.handleKeyDown = this.handleKeyDown.bind(this);
this.handleClick = this.handleClick.bind(this);
this.isSelected = this.isSelected.bind(this);
this.handleRowSelection = this.handleRowSelection.bind(this);
}

handleRowSelection (selectedRows) {
    console.log(selectedRows);
    this.setState({
        selected: selectedRows
    });
}

handleRequestSort(event, property) {
    const orderBy = property;
    let order = 'desc';

    if (this.state.orderBy === property && this.state.order === 'desc') {
        order = 'asc';
    }

    const data = this.state.data.sort(
            (a, b) => (order === 'desc' ? b[orderBy] > a[orderBy] : a[orderBy] > b[orderBy]),
        );

    this.setState({ data, order, orderBy });
}

handleSelectAllClick(event, checked) {
    if (checked) {
        // this.setState({ selected: this.state.data.map(n => n.id) });
        // return;
        this.setState({isSelected: true});
    }
    this.setState({ selected: [] });
}

handleKeyDown(event, id) {
    if (keycode(event) === 'space') {
        this.handleClick(event, id);
    }
}

handleClick(event, id) {
    const { selected } = this.state;
    const selectedIndex = selected.indexOf(id);
    let newSelected = [];

    if (selectedIndex === -1) {
        newSelected = newSelected.concat(selected, id);
    } else if (selectedIndex === 0) {
        newSelected = newSelected.concat(selected.slice(1));
    } else if (selectedIndex === selected.length - 1) {
        newSelected = newSelected.concat(selected.slice(0, -1));
    } else if (selectedIndex > 0) {
        newSelected = newSelected.concat(
            selected.slice(0, selectedIndex),
            selected.slice(selectedIndex + 1),
        );
    }

    this.setState({ selected: newSelected });
}

isSelected(id) {
    this.state.selected.indexOf(id) !== -1;
}

render() {
    const classes = this.props.classes;
    const { data, order, orderBy, selected, selectable, multiSelectable, showCheckboxes, enableSelectAll } = this.state;

    return (
        <Paper className={classes.paper}>
            <EnhancedTableToolbar numSelected={selected.length} />
            <Table>
                <EnhancedTableHead
                    numSelected={selected.length}
                    order={order}
                    orderBy={orderBy}
                    onSelectAllClick={this.handleSelectAllClick}
                    onRequestSort={this.handleRequestSort}
                    displaySelectAll={showCheckboxes}
                    adjustForCheckbox={showCheckboxes}
                    enableSelectAll={enableSelectAll}
                />
                <TableBody multiSelectable={multiSelectable}
    selectable={selectable}
        >
                    {data.map(n => {
                        const isSelected = this.isSelected(n.id);
                        return (
                            <TableRow
                                hover
                                onClick={event => this.handleClick(event, n.id)}
                                onKeyDown={event => this.handleKeyDown(event, n.id)}
                                role="checkbox"
                                aria-checked={isSelected}
                                tabIndex={-1}
                                key={n.id}
                                selected={isSelected}
                            >
                                <TableCell checkbox>
                                    <Checkbox checked={isSelected} />
                                </TableCell>
                                <TableCell disablePadding>{n.type}</TableCell>
                                <TableCell numeric>{n.name}</TableCell>
                                <TableCell numeric>{n.DataID}</TableCell>
                            </TableRow>
                        );
                    })}
                </TableBody>
            </Table>
        </Paper>
    );
}

}

EnhancedTable.propTypes = {
classes: PropTypes.object.isRequired,
};

export default withStyles(styles)(EnhancedTable);