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 bulk delete for devices browser #4219

Merged
merged 17 commits into from
Jul 24, 2024
Merged

Add bulk delete for devices browser #4219

merged 17 commits into from
Jul 24, 2024

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jul 19, 2024

closes #4218

Description

  • Adds support for checkbox column to table component
  • Enables checkboxes in devices browser
  • Adds Delete Bin icon in device browser toolbar

Demo

chrome_8Wx6xgUHun

TODO:

  • Fix Checkbox styling in table (@joepavitt )
  • Tests
  • Docs

Related Issue(s)

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.16%. Comparing base (3f397c3) to head (f591f88).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4219      +/-   ##
==========================================
- Coverage   78.66%   78.16%   -0.51%     
==========================================
  Files         286      287       +1     
  Lines       13161    13261     +100     
  Branches     2949     2965      +16     
==========================================
+ Hits        10353    10365      +12     
- Misses       2808     2896      +88     
Flag Coverage Δ
backend 78.16% <ø> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Steve-Mcl
Copy link
Contributor Author

@joepavitt this is now ready to switch out of draft - please pull and update your local branch and switch out of draft when you have had a moment to sort out the funky column sizing.

@Steve-Mcl Steve-Mcl changed the base branch from main to 4208-bulk-device-delete-api July 19, 2024 14:38
@Steve-Mcl
Copy link
Contributor Author

@joepavitt if you havent already pulled this local ignore.

I had this targeting main - I just re-targeted it as it was based off the API - you might want to pull local once more (sorry)

@knolleary
Copy link
Member

How scalable is listing all of the devices in the dialog? What happens if they have 50 devices to delete?

@joepavitt
Copy link
Contributor

Yeah, I'd say just say "Are you sure you want to delete N devices" - no need to list them explicitly.

@Steve-Mcl
Copy link
Contributor Author

Yeah, I'd say just say "Are you sure you want to delete N devices" - no need to list them explicitly.

I disagree. I explicitly want to spell out what is to be deleted to avoid ambiguity (e.g. when a filter is applied AFTER selection). I've been stung in other applications like this.

However, Nick was right to point out scalability of large number of devices - currently this is what a user would see:

image

However, with a simple parent div with max-height and auto scroll overflow, we can still show devices but in a contained manor.

image

@Steve-Mcl
Copy link
Contributor Author

How scalable is listing all of the devices in the dialog? What happens if they have 50 devices to delete?

contained with a max-height auto scroll parent div in contain device delete list to sensible size then scroll

Base automatically changed from 4208-bulk-device-delete-api to main July 24, 2024 09:01
@Steve-Mcl
Copy link
Contributor Author

@joepavitt In an effort to move this along, do you want me to investigate the funky sizing on the new checkbox column and the existing kebab menu column?

@joepavitt
Copy link
Contributor

Sorry Steve, this completely slipped off my radar. Will ping you on Slack

@joepavitt joepavitt marked this pull request as ready for review July 24, 2024 10:58
@joepavitt
Copy link
Contributor

joepavitt commented Jul 24, 2024

Functionally great, I've pushed fixes for the CSS alignment and table width problems, although seeing:

Screenshot 2024-07-24 at 11 59 31

in the console. The problem is caused by <ff-checkbox v-model="checks[row[checkKeyProp]]" /> equating to undefined, as we need to ensure we've mapped the relevant rows into checks[] to at least show as false.

Leave it with me, and I'll sort.

@Steve-Mcl
Copy link
Contributor Author

Screenshot 2024-07-24 at 11 59 31 in the console. The problem is caused by `` equating to `undefined`, as we need to ensure we've mapped the relevant rows into `checks[]` to at least show as `false`.

I seen these too. I didnt get around to reading up on this - my approach was going to be to look for something like an "ignore" or "infer" directive for vue to ignore these warnings. Effectively, we don't really care. So long as "not true" can implicitly mean false (we dont care that it is undefined/null/whatever)?

@joepavitt joepavitt merged commit 6ef2608 into main Jul 24, 2024
13 of 14 checks passed
@joepavitt joepavitt deleted the 4218-bulk-delete-ui branch July 24, 2024 12:57
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.

Add multiple selection and bulk deletion to Device Browser
3 participants