-
Notifications
You must be signed in to change notification settings - Fork 84
Add UI for custom taxonomy CRUD #6921
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Adds UI functionality for creating, editing, and deleting custom taxonomies in the taxonomy management page.
Key Changes:
- New components for custom taxonomy CRUD operations:
CreateCustomTaxonomyForm,CustomTaxonomyDetails, andCustomTaxonomyEditDrawer - Added RTK Query mutations (
createCustomTaxonomy,updateCustomTaxonomy,deleteCustomTaxonomy) with proper cache invalidation - Updated taxonomy tree to support custom root node labels and click handlers for custom taxonomies
- Renamed
TaxonomyEditDrawertoTaxonomyItemEditDrawerfor clarity between taxonomy items vs taxonomy types - Added "+ Create new" option to the floating menu for creating custom taxonomies
- Added TypeScript types for taxonomy CRUD operations
Issues Found:
- Drawer doesn't close after successful taxonomy deletion - needs to call
onClose()inhandleDeletefunction
Confidence Score: 3/5
- This PR is mostly safe to merge but has a UX bug that needs fixing
- The implementation is well-structured with proper separation of concerns, correct use of RTK Query patterns, and appropriate cache invalidation. However, there's a logical bug where the drawer remains open after successfully deleting a taxonomy, which creates a poor user experience. The bug is straightforward to fix but should be addressed before merging.
- Pay close attention to
CustomTaxonomyEditDrawer.tsxwhich has the delete flow bug
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| clients/admin-ui/src/features/taxonomy/components/CreateCustomTaxonomyForm.tsx | 5/5 | New component for creating custom taxonomies with name and description fields |
| clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyEditDrawer.tsx | 3/5 | New drawer component for editing and deleting custom taxonomies, missing drawer close on successful delete |
| clients/admin-ui/src/features/taxonomy/taxonomy.slice.ts | 5/5 | Added RTK Query mutations for creating, updating, and deleting custom taxonomies with proper cache invalidation |
| clients/admin-ui/src/pages/taxonomy/index.tsx | 4/5 | Main taxonomy page updated with create/edit/delete flows for custom taxonomies and modal for creating new taxonomies |
13 files reviewed, 1 comment
clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyEditDrawer.tsx
Outdated
Show resolved
Hide resolved
| }, [taxonomyType, isPlusEnabled]); | ||
|
|
||
| const toast = useToast(); | ||
| const [messageApi, messageContext] = message.useMessage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to have the message API at this level so the toasts don't get closed prematurely when the drawer and modal close.
speaker-ender
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Mostly minor changes.
Let me know if you have any questions
clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyDetails.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyEditDrawer.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyEditDrawer.tsx
Outdated
Show resolved
Hide resolved
| > | ||
| Are you sure you want to delete this taxonomy? This action cannot be | ||
| undone. | ||
| </Modal> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do this with the modal api confirm?
We could also move up out of the drawer content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on confirm. Is there a particular reason you prefer it outside the drawer? In general I usually like modals to be together with the thing that triggers them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't visually or conceptually match what is happening when a modal is rendered in the dom.
It's independent from the drawer visually and it shouldn't need to have the drawer rendered to exist.
ex. If we allow for the same action to be performed by a button outside of the drawer, it should work the same way and trigger the same modal.
clients/admin-ui/src/features/taxonomy/components/CustomTaxonomyDetails.tsx
Show resolved
Hide resolved
…om/ethyca/fides into jpople/eng-1261/custom-taxonomy-ui
Ticket ENG-1261
Description Of Changes
Adds UI for adding and updating custom taxonomies.
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works