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

[Table] onRowSelection Bug #1897

Closed
husek opened this issue Oct 16, 2015 · 22 comments
Closed

[Table] onRowSelection Bug #1897

husek opened this issue Oct 16, 2015 · 22 comments
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module!

Comments

@husek
Copy link

husek commented Oct 16, 2015

Evening guys,

Let's say i have the following table:

                        <Table
                            height='100%'
                            selectable={true}
                            multiSelectable={true}
                            onRowSelection={this._onRowSelection}>
                            >

If i do something like this, it'll work fine:

    _onRowSelection: function(selectedRows){
        console.log(selectedRows);
    },

but if i attach any kind of more complex event, it'll make impossible to 'check' ANY other rows.
here's some examples of events:

    _onRowSelection: function(selectedRows){
        this.context.flux.getActions('conversation').setSelectedRows(selectedRows);
    },

    _onRowSelection: function(selectedRows){
       this.setState({
        selected: selectedRows.slice(0);
      });
    },

Both of them (Flux and non-flux version) cause the same issue.

Just to clarify it just freeze all my rows as unchecked, and if i put an console.log with the setState, it's returning just the last selected row instead of all rows.

any idea guys?

@andrewhn
Copy link

The table 'resets' on the render method being (re)called. You can see this if you wrap the setState in your _onRowSelection method in a timeout.

To get around it, you can do something like this:

// on row selection event handler like this
_onRowSelection(rows) {
  this.setState({selectedRows: rows});
},

// when you generate your table body, set the 'selected' attribute of each row based
// on the list you're keeping in state
<TableBody>
  {this.state.rowDataList.map((d, i) => {
    return <TableRow key={i}
                     selected={this.state.selectedRows.indexOf(i) !== -1}>
      <TableRowColumn>
        {d.attr}
      </TableRowColumn>
    </TableRow>
  })}
</TableBody>

@husek
Copy link
Author

husek commented Oct 26, 2015

@andrewhn,

Hi Andrew,
Thanks for your answer, however, i still get the same results as described before.

Any ideas?

@andrewhn
Copy link

@husek Does row selection work briefly when you change your _onRowSelection code to the following?

_onRowSelection: function(selectedRows){
   setTimeout(function() {
     this.setState({
      selected: selectedRows.slice(0);
     });
   }, 2000);
},

@husek
Copy link
Author

husek commented Oct 26, 2015

@andrewhn , Yes, it does work for a short time with this snippet.

@andrewhn
Copy link

@husek So you see that when render() is called, your table is 'resetting' to having no rows selected. To get around this, you need to set the selected attribute of the TableRow components yourself, as in my example above.

@husek
Copy link
Author

husek commented Oct 27, 2015

ahd strange thing, @andrewhn , it's that i set selected at TableRow, but still doesn't working after the render 'reset', here's an simplified version of my component:

    getInitialState: function(){
        return {
            offset: 0,
            selectedRows: []
        }
    },


    _onRowSelection: function(selectedRows){
        setTimeout(function() {
            this.setState({
                selectedRows: selectedRows.slice(0)
            });
        }, 2000);
    },




    render: function () {
        console.log(this.state.selectedRows);

        return (
            <div>
                <Table
                    height='100%'
                    selectable={true}
                    multiSelectable={true}
                    onRowSelection={this._onRowSelection}
                    >
                    <TableBody
                        ref='currentTable'
                        deselectOnClickaway={false}
                        multiSelectable={true}
                        preScanRows={false}
                        showRowHover={true}
                        stripedRows={false}
                        >

                        {this.props.conversations.results.get('list').map(function(item, i){
                            return (
                                <TableRow key={i}  selected={this.state.selectedRows.indexOf(i) !== -1}>
                                    <TableRowColumn colSpan="1"><LogoGenerator integration={item.int_key}/></TableRowColumn>
                                    <TableRowColumn colSpan="2">[COSTUMER_MARKUP]</TableRowColumn>
                                    <TableRowColumn colSpan="3" >
                                        <Link to='Chat' params={{ id : item.id || '' }}>
                                            <strong>{item.title}</strong><br/>
                                            <span>{item.latest_content_repr}</span>
                                        </Link>

                                    </TableRowColumn>
                                    <TableRowColumn colSpan="3">
                                        <Link to='editItem' params={{ id : linkGenerator(item, 'id') || '' }}>
                                            {linkGenerator(item, 'name')}
                                        </Link>
                                    </TableRowColumn>
                                    <TableRowColumn colSpan="2">{Moment(item.latest_message_time).format('DD/MM/YYYY [-] HH:MM:SS')}</TableRowColumn>
                                </TableRow>
                            )
                        }.bind(this))}

                    </TableBody>
                </Table>

            </div>
        );
    }
});

@alitaheri alitaheri added the bug 🐛 Something doesn't work label Dec 9, 2015
@vmaudgalya
Copy link
Contributor

This code snippet will work for a single checked box, and it can be extended for multiple checked boxes by using an array as described above by @andrewhn


  _onRowSelection(selectedRows) {
    let rowNumber = selectedRows[0]
    this.setState({
      selectedRow: rowNumber,
    })
  },

  render() {
    let items = this.state.items.map((item, index) => {
      return (
        <TableRow key={item.id} selected={this.state.selectedRow === index}>
          <TableRowColumn>{item.name}</TableRowColumn>
        </TableRow>
      )
    })
  }

One possible fix for this issue would involve the library owner following a similar approach, persisting all checked rows and rebuilding them as checked rows when render() is called, and subsequently deleting the persisted checked rows in componentWillUnmount()

@szalansun
Copy link

Hi ,
Any plan on fixing this issue?
I get the same problem...

@Ymatigoosa
Copy link

Ymatigoosa commented Jun 3, 2016

Even if you set <TableRow key={item.id} selected={true}> you can remove selection by clicking on the row. Is seems there is no way to make a controlled table widget now.

@corytheboyd
Copy link

corytheboyd commented Jun 6, 2016

This bug makes the Table Material UI element completely unusable for elements that want to use it dynamically, I am disappoint :( Specifically, I wanted to use a multi-select Table for adding many items in a collection (this.props.collection) with an "Add all selected" button, utilizing the onRowSelection callback of the Table component to alter local state. Unfortunately the table does to maintain state across renders-- which is totally okay, but there isn't a way for the TableRow components to re-compute their selected values on re-render. I propose that an optional isSelected func prop be added to TableRow so we can make this almost awesome Table work in our applications :D

Edit: The TableRow component should be changed, not the TableRowColumn component.

@corytheboyd
Copy link

corytheboyd commented Jun 6, 2016

Threw together an example component with my proposed optimistic functionality, closely mimicing code I wrote before encountering this bug:

import React, { Component, PropTypes } from 'react';

import {
  Table,
  TableHeader,
  TableHeaderColumn,
  TableBody,
  TableRow,
  TableRowColumn
} from 'material-ui';
import ContentAddIcon from 'material-ui/svg-icons/content/add';

class MyComponent extends Component {
  constructor (props) {
    super(props);
    this.state = {
      selectedPeople: []
    };
  }

  handleRowSelection (indices) {
    if (indices === 'all') {
      this.setState({ selectedPeople: this.props.people });
    } else if (indices === 'none') {
      this.setState({ selectedPeople: [] });
    } else {
      let people = indices.map(index => {
        return this.props.people[index];
      })
      this.setState({ selectedPeople: people })
    }
  }

  isPersonSelected (subjectPerson) {
    this.state.selectedPeople.forEach(selectedPerson => {
      if (subjectPerson.id === selectedPerson.id)
        return true;
    });
    return false;
  }

  doSomethingWithAllSelectedPeople () {
    // Whatevs
  }

  render() {
    var doSomethingButton = null;
    if (this.state.selectedPeople.length > 0) {
      doSomethingButton = (
        <FloatingActionButton onMouseUp={this.doSomethingWithAllSelectedPeople.bind(this)}>
          <ContentAddIcon />
        </FloatingActionButton>
      );
    }

    return (
      <div className="my-component">
        <Table
          selectable={true}
          multiSelectable={true}
          onRowSelection={this.handleRowSelection.bind(this)}
        >
          <TableHeader>
            <TableRow>
              <TableHeaderColumn>ID</TableHeaderColumn>
              <TableHeaderColumn>Name</TableHeaderColumn>
            </TableRow>
          </TableHeader>
          <TableBody>
            {this.props.people.map((person, index) => {
              return (
                <TableRow key={index} isSelected={this.isPersonSelected.bind(this, person)}>
                  <TableRowColumn>{person.id}</TableRowColumn>
                  <TableRowColumn>{person.name}</TableRowColumn>
                </TableRow>
              )
            })}
          </TableBody>
        </Table>
        {doSomethingButton}
      </div>
    );
  }
};

MyComponent.propTypes = {
  people: PropTypes.arrayOf(PropTypes.shape({
    id: PropTypes.number,
    name: PropTypes.string
  }))
};

export default MyComponent;

@safouman
Copy link

safouman commented Jun 8, 2016

Any plan on fixing this issue?

@husek
Copy link
Author

husek commented Jun 8, 2016

Well, i have opened this issue almost a year ago, and so far i've used two different approaches to fix this problem:

  1. Forked the project and added this method into the /table/table-body.js:
  getValue: function getValue() {
    if (this.props.allRowsSelected) {
      return 'all'
    } else {
      return this.state.selectedRows
    }
  }

So i was able to do something like this.refs.theTable.getValue();

  1. Not sure why, but this started to work fine after the 0.14 release:
<Table
                                    height='100%'
                                    selectable={true}
                                    multiSelectable={true}
                                    onRowSelection={this.handleRowSelect}
                                    >
 <TableHeader enableSelectAll={true}>[yadda yadda]</TableHeader>
                        <TableBody
                                        ref='currentTable'
                                        deselectOnClickaway={false}
                                        multiSelectable={true}
                                        showRowHover={true}
                                        stripedRows={false}
                                        >
{// Your content.map
 <TableRow key={i}  selected={this.state.selectedRows.indexOf(i) !== -1}> </TableRow>}
                                    </TableBody>

and

    handleRowSelect: function(newArray){
            var result = newArray.slice(0);
            if (result == 'none') {
                result = [];
            }
            this.setState({selectedRows: result})
        }

@corytheboyd
Copy link

Huh, not sure what fever dream I was in when I posted because I just got it to work too. I think my selected={} function was just busted. D'oh!

@corytheboyd
Copy link

@husek Maybe it's time to close this issue :)

@husek
Copy link
Author

husek commented Jun 11, 2016

@corytheboyd, indeed it's!

(But i still think that the getValue() method should be part of the lib 😄 )

@husek husek closed this as completed Jun 11, 2016
@rachel-singh
Copy link

rachel-singh commented Dec 21, 2016

I am using version 0.16.4 and still seeing this issue. I dynamically set the selected property to true before the table is rendered but the checkboxes do not get selected.

var rows = [];
        this.props.cashLineItems.forEach(function (cli) {
            if (cli) {
                var isSelected = false;
                this.state.userSelectedRows.forEach(function(row) {
                    if (row.cashLineItemId === cli.cashLineItemId)
                        isSelected = true;
                });
                rows.push(
                    <TableRow key={cli.cashLineItemId} selected={isSelected}>
                        <TableRowColumn>{moment(cli.accountingDate).format("MM/DD/YYYY")}</TableRowColumn>
                        <TableRowColumn>{cli.accountCode}</TableRowColumn>
                        <TableRowColumn>{cli.ledgerAccount}</TableRowColumn>
                        <TableRowColumn>{cli.description}</TableRowColumn>
                        <TableRowColumn/>
                    </TableRow>
                );
            }
        }, this); 

Any ideas? Thanks.

@ugomeda
Copy link
Contributor

ugomeda commented Jan 31, 2017

@rachel-singh : I found a way to hack this behaviour. Instead of managing the selected property of the rows, use a ref to your TableBody and modify its state. This updates the selected state of the rows and does not fire a onRowSelection event :

<TableBody ref={(tableBody) => { this.tableBody = tableBody; }}>

Then in your handler :

this.tableBody.setState({ selectedRows: selection });

It would be nice to have a documented and tested way to implement this though.

@sc6
Copy link

sc6 commented Feb 28, 2017

Anyone know why the above works?

@cristianoliveira
Copy link

I am still having the same problem :/

@spoeck
Copy link

spoeck commented Jun 10, 2017

I used the solution of ugomeda. Important is that you set the state of the TableBody after you set your state. To achieve that, I set the state of the TableBody as callback of my own state. Following works for me:

    constructor() {
        super();
        this.state = {
            selectedRows: []
        };

        this._onRowSelection = this._onRowSelection.bind(this);
    }


    render() {
        return (
                <Table multiSelectable onRowSelection={this._onRowSelection}>
                    <TableHeader>
                        <TableRow o>
                            <TableHeaderColumn>c1</TableHeaderColumn>
                            <TableHeaderColumn>c2</TableHeaderColumn>
                            <TableHeaderColumn>c3</TableHeaderColumn>
                        </TableRow>
                    </TableHeader>
                    <TableBody showRowHover ref={(tableBody) => { this.tableBody = tableBody; }}>
                        {this._renderTableRow()}
                    </TableBody>
                </Table>
        );
    }

    _onRowSelection(rows: Array<number>) {
        this.setState({selectedRows: rows}, () => this.tableBody.setState({ selectedRows: rows }));
    }

@BenRichter
Copy link

BenRichter commented Aug 3, 2018

Another late solution for arrow functional style: Using "pure" from recompose
https://github.com/acdlite/recompose

import { compose, pure, setDisplayName } from 'recompose';

const enhance = compose(
  setDisplayName('MailAttachments'),

  setPropTypes({
    onRowSelection: PropTypes.func.isRequired
  }),

  pure
)

export default enhance(  ({ onRowSelection }) => (
    <Table selectable onRowSelection={onRowSelection}>
         ...
   </Table>
)  )

@oliviertassinari oliviertassinari added the component: table This is the name of the generic UI component, not the React module! label Jul 30, 2020
mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests