carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://ibm-products.carbondesignsystem.com
Apache License 2.0
98 stars 138 forks source link

[Datagrid]: SelectAll not working as expected with virtual scrolling on #6125

Open flannanl opened 1 month ago

flannanl commented 1 month ago

Package

Carbon for IBM Products

Description

When clicking the Select All checkbox available in the header row, not all the rows are selected. It only selects the rows that are visible in the current viewport. This is a regression introduced by https://github.com/carbon-design-system/ibm-products/pull/5933. If I disable virtual scrolling, the Select All function is then working as expected.

Component(s) impacted

By inspecting the code, this is caused by this line: https://github.com/carbon-design-system/ibm-products/blob/752739bde31c3dcb5de1ff2702167b7a838350dd/packages/ibm-products/src/components/Datagrid/Datagrid/addons/stateReducer.js#L170, where the 3 rows not selected in the video below do not have getRowProps function.

select-all-virtual-scrolling

Browser

Chrome, Safari, Firefox, Microsoft Edge

@carbon/ibm-products (previously @carbon/ibm-cloud-cognitive) version

v2.50.0

Suggested Severity

Severity 2 = Aspects of design is broken, and impedes users in a significant way, but there is a way to complete their tasks. Affects major functionality, has a workaround.

Product/offering

IBM Cloud Projects

CodeSandbox or Stackblitz example

N/A

Steps to reproduce the issue (if applicable)

No response

Release date (if applicable)

No response

Code of Conduct

devadula-nandan commented 1 month ago

Hi @flannanl,

I will discuss this in detail with our team to triage this. meanwhile here is some information about this functionality. https://pages.github.ibm.com/carbon/ibm-products/components/datagrid/batch-action/usage/#actions-across-multiple-pages

const TOGGLE_ROW_SELECTED = 'toggleRowSelected'; // selects individual row
const TOGGLE_ALL_ROWS_SELECTED = 'toggleAllRowsSelected'; // selects all rows in entire table
const TOGGLE_ON_PAGE_ALL_ROWS_SELECTED = 'toggleAllRowsOnPageSelected'; // selects all rows in the current page

so the select all checkbox is supposed to TOGGLE_ON_PAGE_ALL_ROWS_SELECTED in the current view. previously it used to select rows in the entire table.

devadula-nandan commented 1 month ago

hi @flannanl,

were you able to utilize the select all from the batch action toolbar?

As per my team discussion, this would be the intended behaviour, and the usage docs mentions to use select all button from the batch action toolbar, you also have an option to remove the checkbox at the top of the header. but the checkbox behaviour is meant to select only the current view

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 7 days with no activity. Remove the stale label or add a comment, otherwise this issue will be closed in 7 days.

flannanl commented 1 month ago

@devadula-nandan if it is a paginated table, it makes sense. For a table that can scroll, that does not really make sense. As a user, they would not know if the table is using virtual scrolling or not. All they see is a table with a scrollbar that they will expect as they scroll, they will see the rest of the rows. When clicking Select All, everyone would expect all the rows in that table, whether it is visible or not, will be selected. I can leave it as is. But if our users start complaining, I am going to come to you again. Since this is also a visualization issue, adjusting the batch actions would not help. Also as we can have 50 or even 100s of items, I do not want to disable Select All.

vsnichols commented 1 month ago

@devadula-nandan @matthewgallo Would love to meet to chat. My understanding is that the Carbon data table component can support BOTH select all and select all on current page when batch actions and pagination are enabled. I think we would be asking to be able to match that behavior, because a user expects to be able to click the "Select all" checkbox in table header and have it select all.

If the backend can support it, the component should be able to handle it. Will grab time to discuss!

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 7 days with no activity. Remove the stale label or add a comment, otherwise this issue will be closed in 7 days.

ljcarot commented 3 weeks ago

Find time to meet - @matthewgallo