iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
92 stars 35 forks source link

[Table] Expandable row selection toggle works incorrectly when there are disabled rows under it #1972

Closed juliusBaranauskas closed 23 hours ago

juliusBaranauskas commented 1 month ago

Describe the bug (current behavior)

Expandable row inside a Table, that has disabled and enabled rows as its' sub-rows handles selection toggling incorrectly.

Expected Behavior

That seems incorrect firstly because disabled rows (in our team's opinion) should not be counted when checking whether all sub-rows are selected, therefore the checkbox state in Step 3 should have been that all sub-rows are selected. Secondly, toggling the selection in Step 4 should in-turn disable all selected rows that are sub-rows of the expandable row.

Link to minimal repro

https://itwin.github.io/iTwinUI/react/?story=table--full2

Steps To Reproduce

  1. Use Table component with selectable rows and expandable rows allowed

  2. Add a row with at least one sub-row enabled and one disabled

  3. Toggle selection of the expandable row (notice a minus icon appear inside the checkbox rectangle, indicating some, not all sub-rows are selected) image

  4. Toggle selection of the same expandable row again (notice that the selection stays the same but minus icon disappears) image

Product 4 row in the repro link can be used as an example.

Anything else?

No response

mayank99 commented 4 weeks ago

I think this issue might be the same as #928?

juliusBaranauskas commented 4 weeks ago

@mayank99 seems similar in some regards of the issue, although in that case the reason for why not all sub-rows are selected is because they are filtered out, however they are still selectable. Where I see the difference - in my case the rows are not even selectable so I'd argue that since all of the selectable rows are selected, the parent row selection should indicate that they are all selected, whereas in the issue you mentioned some rows are just hidden, not selected, but they are still selectable (and some might even be selected off-screen before filtering happened) so the initial state seems correct for that scenario.

mayank99 commented 4 weeks ago

It's unlikely that we will change the parent row to be (visually) selected when some rows are not selected. It might fit your narrow use-case (in which case I think you might be able to set manual state?), but our Table is designed to support a wide variety of scenarios. Think of a case where some disabled rows might be pre-selected; these should be taken into account when calculating the total selected state.

The main "bug" here is only that clicking on the checkbox does nothing, similar to #928 (but maybe not the same). We'll look into it 👍 Thanks for the report.

I also noticed some weird behaviors around when the row is expanded vs collapsed. This is all likely caused by improper memoization conditions in TableRowMemoized.

https://github.com/iTwin/iTwinUI/assets/9084735/7b3260c0-387d-4498-bd3b-6769341cac8f

idhard commented 4 weeks ago

hey i just want to chip in. It looks like the selector stop working only when the sub rows contains disabled values. Otherwise it works

mayank99 commented 3 weeks ago

@idhard Can you elaborate on what you mean by "selector"? Maybe you can share a screenshot/screen-recording of what you're seeing vs what you expected to see.

idhard commented 3 weeks ago

@mayank99

https://github.com/iTwin/iTwinUI/assets/820664/96c19c95-9f8b-4bcc-9163-88000df9adf8

the bug only happen when the parent rows contain disabled child rows, otherwise the checkbox seems to work

mayank99 commented 14 hours ago

This has been fixed in 3.10.0 🥳