-
Notifications
You must be signed in to change notification settings - Fork 72
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
Migrate custom selects (Chakra -> Ant) #5502
Conversation
* Visit System Inventory where an integration has been configured (Iterable) * Ensure the Request types multi-select works as expected and the form saves.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #11139
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5502/merge
|
Run status |
Passed #11139
|
Run duration | 00m 37s |
Commit |
4a2835dfd4 ℹ️: Merge 016b2f01f7df725ed74125e8d2582554a1e28277 into d1baca99b58d9c13d5e99d23a94f...
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
b56aa85
to
1f10946
Compare
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.
Code looks good-- UAT'd as described in PR and confirming everything works.
I am seeing one bug on the connection manager page where the "Connection type" filter select doesn't render options, not sure if that was introduced with this PR or missed on the earlier one:
Also, after clicking "add connection", this ConnectionTypeFilter component appears to still be using the old, bespoke SelectDropdown component, could we switch that over to Ant as well? It seems to be the last one-- its counterpart MultiSelectDropdown is also still in the codebase but isn't used anywhere anywhere and should be able to be removed.
@@ -35,7 +35,6 @@ | |||
"@reduxjs/toolkit": "^1.9.3", | |||
"@tanstack/react-table": "^8.10.7", | |||
"@types/jest": "^29.5.12", | |||
"chakra-react-select": "^3.3.7", |
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.
🎉
return ( | ||
<Flex alignItems="center" mb={6} {...props}> | ||
<Button | ||
onClick={() => nextRouter.push(backPath)} |
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.
What's the reasoning for this change?
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 question, I meant to leave a comment on this PR about it and forgot...
This was more of an annoyance fix. The link part would work as expected in a SPA where the page would immediately load. This button part however would load the entire app again. This change just aligns the button behavior with the link behavior using the Next router.
@jpople Thanks for finding that one-off select. It helped me find 2 more in the Privacy Requests table filter! |
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! Thanks for getting those extra old selects, it's great to have that cleanup done.
fides Run #11156
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #11156
|
Run duration | 00m 38s |
Commit |
43b434b6be: Migrate custom selects (Chakra -> Ant) (#5502)
|
Committer | Jason Gill |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Closes HJ-160
Description Of Changes
Code Changes
colorPrimaryBg
topalette.FIDESUI_NEUTRAL_75
as per @jack-gale-ethyca's requestSteps to Confirm
/consent/configure
)/settings/custom-fields
)/add-systems
)/consent/privacy-notices/new
)/integrations
)/settings/organization
)/consent/privacy-experience/new
)/privacy-requests
)/privacy-requests/configure/storage
)/properties
)/reporting/datamap
)/taxonomy
)/datastore-connection
)Pre-Merge Checklist
CHANGELOG.md
updatedmain
downgrade()
migration is correct and works