[TableBody] revert willReceiveProps to respect selected property on TableRow

[jnishiyama]

This PR regards issue #6006. TableRow does not respect selected property, making it impossible to programmatically select a row. The code in this PR is the same code that was present as late as version 0.16.6, and resolves the issue.

N.B. Props where props are due: @mquandvr found the fix, but I decided to make the PR, because I am in need of this feature working.

[jnishiyama]

Apologies for all the commits...

[oliviertassinari]

@jnishiyama Thanks for the PR, unfortunately it's breaking a unit test. You would have to fix it and add a new one for your use case if you want to move forward with that PR. I don't know much about the internal of the Table component.

[mbrookes]

Props where props

@jnishiyama I see what you did there. 😄

[jnishiyama]

@oliviertassinari Hey I understand, but I was wondering if I can remove this test that you committed: the reason being that if the checkbox's checked property is to be invariant on update, then it would seem that one could not programmatically select/deselect the rows.

If the purpose is to prevent a re-render if the default hasn't changed, then one option would be to compare the props, but I'm not sure if that defeats the purpose... e.g.

 componentWillReceiveProps(nextProps) {
    if (this.props.allRowsSelected && !nextProps.allRowsSelected) {
      this.setState({
        selectedRows: [],
      });
    } else {
      const oldRows = this.calculatePreselectedRows(this.props)
      const nextRows = this.calculatePreselectedRows(nextProps)
      if (!shallowEqual(oldRows, nextRows)) {
        this.setState({
          selectedRows: nextRows,
        });
      }
    }
  }

[oliviertassinari]

I can remove this test that you committed

The test was added in order to prevent regression on #5884.
Would you mind adding a test to prevent a future PR like this one trying to address one use case and breaking the others? The logic of the table is quite tricky.

[jnishiyama]

@oliviertassinari I added a test to prevent regression on my commit, and I never removed the test that was added for invariance on update. I did add a shallow compare in the willReceiveProps step, and I modified the test setup a bit so that I could test the change when the props were changed. Please let me know if this makes sense.

[oliviertassinari]

@jnishiyama Thanks for the new test! I have tried running the added test on the master branch and that seems to be green. It looks like you are experiencing another issue.

[jnishiyama]

@oliviertassinari odd, it was failing for me with the code from master. Is the shallowCompare approach acceptable as a fix for the issue it is intended to solve?

[ehourigan]

+1 -- any update on when this might get into master?

[yelrayv]

typo: Mutli -> Multi

[yelrayv]

typo: Mutli -> Multi

[jnishiyama]

@oliviertassinari @yelmu any word on whether or not this implementation is sufficient? Or if there is something I should rework for a fix to be merged?

[mbrookes]

@jnishiyama In the docs 'Complex example', enabling multi-select always selects rows 0 and 2.

[jnishiyama]

@mbrookes that's because in the docs code rows 0 and 2 have their selected property set to true:

const tableData = [ { name: 'John Smith', status: 'Employed', selected: true, }, { name: 'Randal White', status: 'Unemployed', }, { name: 'Stephanie Sanders', status: 'Employed', selected: true, },

[mbrookes]

@jnishiyama That would do it! 😆

I noticed a change in behaviour but didn't look any further. The tableData should probably be in state and updated when changed so that it doesn't reset when this is toggled, but perhaps that's for another PR...

[oliviertassinari]

I have tried running the added test on the master branch and that seems to be green. It looks like you are experiencing another issue.

Sorry, but the introduced tests are green on master. I'm having a hard time understanding what you are trying to fix. Even the demo #6006 keeps the same behavior.

[oliviertassinari]

This change makes that block run after every render, even if the allRowsSelected property didn't change.
That's hiding something wrong. I definitely think that the correct fix isn't has that level of the code.

[jnishiyama]

@oliviertassinari I am trying to fix the fact that you cannot programmatically select rows/checkboxes in the table as it stands (as of when I made this PR). What @mbrookes brought up actually highlights this issue:

In the docs 'Complex example', enabling multi-select always selects rows 0 and 2.

Since the data provided to the table contains data with selected properties being set to true, the table should "select" those rows, however it is not. Similarly, in what is currently in place on the master branch, setting selected to true on table rows does nothing.

As of 0.17.0 (when I made the PR), my test will not pass with the old implementation. I cannot get the tests to run from the most up-to-date master

Further, would you please explain which change specifically you are referring to that causes the re-render, because the logic of the conditional is identical, assuming that the property is boolean. in both implementations of the if logic, allRowsSelected must have changed (since the options are binary) and nextProps.allRowsSelected must evaluate to false. I am having trouble understanding what change causes additional renders. In fact, the implementation I have given avoids renders in the else which were previously occurring unnecessarily.

[oliviertassinari]

@jnishiyama Yes, sorry I was running the unit test, not the integration tests. I have spent too much time on the next branch, I have forgotten we had that setup for the master branch.

Thanks for the explanations! I much better understand the issue now. Actually, I have given a shot at the issue with #6638. Would you mind having a look at it? 😄 . I think that it's fully fixing the #6006 issue.