PF4 Table: 'Select All' checkbox updates #1368
Conversation
| } | ||
|
|
||
| areAllRowsSelected(rows) { | ||
| return rows.every(this.isSelected); |
There was a problem hiding this comment.
Should we skip expanded row children?
It's odd that both parent and child have a selected property because they could be initialized differently and/or be out of sync. In addition, setting the selected property on a child has no affect on its parent.
There was a problem hiding this comment.
I think that fix maybe on your onSelected method, similar to the doc example here -you'll need to weed out the child rows w/o checkboxes in your method, and not set them to row.selected = true.
There was a problem hiding this comment.
If I weed them out myself, will rows.every(this.selected) still work?
There was a problem hiding this comment.
I don't think it's good idea to skip expanded children. There is option to show checkbox for expanded children as well, that might be handy in future once we figure out tree tables.
There was a problem hiding this comment.
Sounds reasonable. Thanks for the explanation
dtaylor113
force-pushed
the
pf4-table-check-all-updates
branch
from
39bdd0f
to
e62b7d7
Compare
Pull Request Test Coverage Report for Build 4449
💛 - Coveralls |
| } | ||
|
|
||
| componentDidUpdate(prevProps) { |
There was a problem hiding this comment.
I don't really like componentDidUpdate function, it could lead to infinite loop (because of calling set state in it's body). Might be better to use static-getderivedstatefromprops.
There was a problem hiding this comment.
After further investigation, I don't think we need to store allRowsSelected in state. Rather send
const headerData = calculateColumns(cells, {
// ...
allRowsSelected: this.props.onSelect ? this.areAllRowsSelected(this.props.rows) : false
})There was a problem hiding this comment.
I don't really like
componentDidUpdatefunction, it could lead to infinite loop
It does have a conditional check, but that might be a better way to go -thanks :-)
| function selectClick(event) { | ||
| const selected = rowIndex === undefined ? event.target.checked : rowData && !rowData.selected; | ||
| onSelect && onSelect(selected, selected, rowId); | ||
| onSelect && onSelect(event, selected, rowId); |
|
This PR calls for new UX design as tri-state checkbo patternfly/patternfly#1411 |
|
PatternFly-React preview: https://1368-pr-patternfly-react-patternfly.surge.sh |
I think that should be handled in a follow on PR |
dtaylor113
force-pushed
the
pf4-table-check-all-updates
branch
from
e62b7d7
to
fdbd17f
Compare
dtaylor113
force-pushed
the
pf4-table-check-all-updates
branch
from
fdbd17f
to
60c89e1
Compare
|
@karelhala and @dlabrecq, I believe this PR is ready, pls review when u get a moment -thanks |
When all rows of the table are selected, the select-all checkbox is now selected automatically. Likewise, when the select-all checkbox is selected, and I deselect one or more rows, the select-all check box is deselected automatically.
Fixes #1352