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

Use CheckboxTree for data map filtering #4864

Merged
merged 4 commits into from
May 8, 2024

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented May 4, 2024

Closes PROD-1916

Description Of Changes

Update the filters on the table to apply a modified version of the hierarchy component that we are using on the dataset page.

Code Changes

  • New DatamapReportFilterModal which reduces a lot of over-engineered complexity and implements the CheckboxTree component
  • Update old getQueryParamsFromList as new getQueryParamsFromArray utility
  • Update checkbox tree to have animated caret similar to accordion.
  • Unit test and e2e test

Steps to Confirm

  • visit Data Map Report /reporting/datamap
  • click the "Filter" button in the top bar
  • expand one of the 3 categories
  • Note the new style for checking boxes uses our existing tree component rather than just a flat list of check boxes within each of the 3 main sections
  • Expand/collapse and click around.
  • When desired filter is selected, click the "Done" button. This will apply the filter to the table.
  • Open the filter modal again using the "Filter" button.
  • Click the "Reset filters" button which will clear all filters.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Copy link

vercel bot commented May 4, 2024

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

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 10:53pm

Copy link

cypress bot commented May 4, 2024

Passing run #7628 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 289630a into 0e27b21...
Project: fides Commit: a5edc3e913 ℹ️
Status: Passed Duration: 00:33 💡
Started: May 7, 2024 11:02 PM Ended: May 7, 2024 11:03 PM

Review all test suite changes for PR #4864 ↗︎

@gilluminate gilluminate marked this pull request as ready for review May 7, 2024 17:43
Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

Tested this, and it works great! Couple small requests below.

Also here's some things I've noticed that I'm unsure if caused by this PR, but would like to double check:

  1. The vertical spacing between items within the accordions seems uneven:
Screenshot 2024-05-07 at 5 45 20 PM
  1. The group_by query param seems to have some encoding issues in the resulting API call:
Screenshot 2024-05-07 at 5 49 43 PM

Let me know and let's re-ticket items as needed!

clients/admin-ui/src/features/common/utils.ts Outdated Show resolved Hide resolved
@gilluminate
Copy link
Contributor Author

  1. The vertical spacing between items within the accordions seems uneven:

I think that was existing, but it's an easy fix. Great catch.

@gilluminate
Copy link
Contributor Author

  1. The group_by query param seems to have some encoding issues in the resulting API call:

That's also existing behavior, and I'm worried about breaking it for the backend. Might be best to re-ticket that one as you suggested.

Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

Mind creating a new ticket for the query param formatting?

@gilluminate gilluminate merged commit 0e11683 into main May 8, 2024
13 checks passed
@gilluminate gilluminate deleted the PROD-1916-update-filters-in-new branch May 8, 2024 15:02
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