angular-ui/ui-grid

selectAll and selectedCount Inconsistencies

Open

#4,495 opened on 2015年10月8日

GitHub で見る
 (2 comments) (0 reactions) (0 assignees)JavaScript (5,395 stars) (2,496 forks)batch import
good first issue

説明

I've noticed in the code base that the grid.selection.selectAll property is not set consistently across different modes of selection.

When searching the code-base, selectAll property is modified:

  • selectAllRows = true
  • selectAllVisibleRows = true
  • toggleRowSelection = based on whether grid.rows.length === selectedRows.length;
  • clearSelectedRows = false

It seems that other selection methods make heavy use of toggleRowSelection. However, shiftSelect does not: https://github.com/angular-ui/ui-grid/blob/fe00489/src/features/selection/js/selection.js#L502

So it's possible that by shift-selecting, selectAll is not properly updated.

Furthermore, I noticed that selectedCount is only updated in GridRow.setSelected, except it's also updated to 0 in clearSelectedRows: https://github.com/angular-ui/ui-grid/blob/fe00489/src/features/selection/js/selection.js#L560

But clearSelectedRows calls setSelected on all the selectedRows so shouldn't that take care of it? Or is https://github.com/angular-ui/ui-grid/blob/fe00489/src/features/selection/js/selection.js#L560 just a fail safe?

Finally, it seems to me that selectAll should not be true when selectAllVisibleRows is called since technically only the visible rows are selected. Was there some motivation to do this? (perhaps paging or virtual scrolling)? I can't seem to find a rationale. Perhaps adding a getVisibleSelectAllState would be more appropriate?

The documentation for getSelectAllState is also somewhat unclear. It states:

----- DOCUMENTATION ----- Returns whether or not the selectAll checkbox is currently ticked. The grid doesnt automatically select rows when you add extra data - so when you add data you need to explicitly check whether the selectAll is set, and then call setVisible rows if it is ----- DOCUMENTATION -----

I'm not sure what the setVisible rows if it is statement actually means as there is not setVisible method. Is this supposed to be setAllVisibleRows()?

コントリビューターガイド