angular-ui/ui-grid

selectAll and selectedCount Inconsistencies

Open

#4,495 opened on Oct 8, 2015

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

Description

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()?

Contributor guide

Tech stack
javascriptangular
Domain
frontend
Issue type
bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
3
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
1-2 days
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
Familiarity with JavaScriptUnderstanding of AngularJSBasic understanding of grid selection logic
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
55
Research direction
Examine the ui grid selection module at src/features/selection/js/selection.js. Verify the behavior of selectAll and selectedCount across all selection methods, paying special attention to shiftSelect which does not use toggleRowSelection. Assess whether selectAll should be set to true when selectAllVisibleRows is called, and consider implementing a getVisibleSelectAllState method. Clarify the documentation for getSelectAllState to fix the unclear reference to setVisible rows.