Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add single select, multi select to QueryTable #1065

Merged
merged 7 commits into from
Jul 22, 2022
Merged

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Jul 22, 2022

Single select

Only one row can be selected at any given time and the selection happens by clicking anywhere in the row. To invoke this mode all you have to do is pass onSelect to the query table.

<Table onSelect={(name) => console.log(name} />

2022-07-22 02 06 38

Multi-select

This is the checkbox selection behavior as we had before but now the whole check cell is a clickable area. To invoke this mode all you have to do is pass onMultiSelect to the query table.

<Table onMultiSelect={(name) => console.log(name} />

2022-07-22 02 07 19


When I was working on #606 I never really felt like the API was right. We pass makeActions to the table because the table actually has to render the row actions menu but the table fundamentally didn't need to know about bulk actions. As I was poking at #1060 today I realized that the logic for what to do when something is selected in the table just needs to live outside the table itself. A callback like onSelect or onMultiSelect is sort of perfect for this. I've updated it so that onSelect just returns the name of the row selected (if any) and onMultiSelect returns a list of names.

This work will unlock both #1060 and #424.

Followups

  • Radio/checkbox background color when the row is hovered is off
  • The single select table needs to allow using the arrow keys to navigate choices

@vercel
Copy link

vercel bot commented Jul 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
console-ui-storybook ✅ Ready (Inspect) Visit Preview Jul 22, 2022 at 9:45PM (UTC)

@zephraph zephraph requested review from david-crespo and benjaminleonard and removed request for david-crespo July 22, 2022 01:21
Comment on lines -119 to +145
const getRowId = useCallback((row) => row.id, [])
const getRowId = useCallback((row) => row.name, [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't believe I didn't think to do this before 😅

@david-crespo
Copy link
Collaborator

david-crespo commented Jul 22, 2022

This is so elegant! It works great. I'm going to take a look at the e2e tests. I also noticed that if you're in single select mode and click the more menu, it also triggers the select. Ideally the click would not propagate up to the row. Probably an easy fix; I'll take a look. But if not it's probably fine for now because we won't use single-select with an actions button anyway.

2022-07-22-select-row

@david-crespo
Copy link
Collaborator

Got it in eddd9da. Just needed onClick={(e) => e.stopPropagation()} on the menu button. I can't think of any problems with that; let me know if you can.

2022-07-22-select-row-fixed

@david-crespo david-crespo merged commit 45abafe into main Jul 22, 2022
@david-crespo david-crespo deleted the boot-disk-selections branch July 22, 2022 22:22
className={cn({ 'cursor-pointer': !!onMultiSelect })}
>
{firstCell.renderCell()}
</UITable.Cell>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing because it puts the onClick on there whether there's a select column or not. The behavior is right because onSingleSelect and onMultiSelect are undefined when they're supposed to be, but it's a little hard to tell from looking it. I think I'll make a followup commit on main.

Copy link
Collaborator

@david-crespo david-crespo Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that in 68adee2. Also renamed onSelect to onSingleSelect in QueryTable because I kept mixing up what was single and what was multi.

onClick={onSingleSelect}
>
<UITable.Cell
onClick={onMultiSelect}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever making the click target the whole cell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants