[IndexTable] Add support for non-selectable table when selectable prop is false #4329
Conversation
Within the IndexProvider the value is accepted as a prop but isn't being used, when it should be. There's already an existing identifier "selectable" which can be renamed as "actionable" because it indicates whether or not actions can be made (are there bulk actions available). Similarly in the useBulkSelectionData hook there's an existing identifier "selectable" where it means "does it currently have any selections. It's a temporary variable created to indicate whether or not any selections have been made, so we rename it to "hasSelections" even though the scope doesn't conflict - just to make it clearer.
This was previously always the case, and when the selectable prop is not provided we would still expect it to default to true, because the main behaviour of this component is for use as a selectable index. This provides backwards compatibility, while still supporting explicitly passing this value as true/false.
Simplifies the closure function because it no longer has multiple return signatures (returning either JSX or JSX[], depending on whether or not its the first element). Splits out a second one that's specifically for rendering the checkbox, and then explicitly calling that and pushing it into the array when selectable.
size-limit report
|
|
Got a good start on this so far, but looks like it'll still need a fair bit of refactoring in the |
|
@ryanwilsonperkin happy to provide support on this if needed! Looking forward to this change. |
|
Hey @ryanwilsonperkin 👋🏽 I didn't realize #4076 existed so I missed that you were already working on this bug! 🤦🏽♀️ I've got a draft PR working as well (#4367), I just need to fix CI failing over the Row tests 👀 |
|
👀 Just a heads up that the Shop channel team is working on a solution to this at #4376 that branches off @chloerice's work |
|
Thanks folks! I'll close this one out in favour of the new PR being created, feel free to re-open if it proves useful. |
4 participants
WHY are these changes introduced?
Fixes #4076
Introduces a variant of the
IndexTablethat has no checkboxes in the leftmost column when theselectableprop is false. This is inline with the naming of the prop and makes the table usable for resources where the user may not need to support selection, and is instead just using the table as a resource-specific version of a DataTable.WHAT is this pull request doing?
selectableprop into the context. This was one of the allowed props on the context but wasn't actually being passed into the context object itself. Instead the context was deriving its own value of "selectable" to mean "there are more than 0 items present" which is renamed to "hasSelections"renderHeadinginline-component to have a simpler signature by separating out a newrenderCheckboxCellinline-component. Previously,renderHeadingwould return a 2-tuple of JSX on the first heading, and a single JSX otherwise, which was a bit confusing and harder to manage when checkbox is optional. Instead we'll do a simple map withrenderHeadingand then insert the checkbox content at the front of the array if desired.A screenshot of the approach rendered with many columns so that it scrolls horizontally. Note the lack of checkbox:
TODO
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste the code from the parent ticket into the Playground in order to test this out.
🎩 checklist
README.mdwith documentation changes