-
Notifications
You must be signed in to change notification settings - Fork 73
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 filtering and pagination to bulk vendor add table #4351
Conversation
Passing run #4964 ↗︎
Details:
Review all test suite changes for PR #4351 ↗︎ |
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.
I've tested locally and things work well for the most part! a thing I noticed that I didn't catch in your initial PR is that the route /add-systems/multiple
is accessible even with the dictionary turned off (i.e. if you visit http://localhost:3000/add-systems/multiple
directly). it's just empty
Perhaps the route shouldn't be available at all (would have to add a flag to nav-config) or add some text about upgrading in that case?
clients/admin-ui/src/features/system/add-multiple-systems/AddMultipleSystems.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/system/add-multiple-systems/AddMultipleSystems.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/common/table/v2/PaginationBar.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/system/add-multiple-systems/MultipleSystemsFilterModal.tsx
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/system/add-multiple-systems/MultipleSystemsFilterModal.tsx
Outdated
Show resolved
Hide resolved
@allisonking Re: route being accessible when the dictionary isn't enabled. I think updating the nav config to handle it makes the most sense. That's what we already do with plus and fides cloud. EDIT: |
ah okay, that might be a bug with how our nested routes work. I remember that bit of code being a bit complex... we might want to revisit this in the future |
the request is happening. This was causing this to redirect if the | ||
request didn't finish fast enough. | ||
*/ | ||
if (!dictionaryService && health !== EMPTY_PLUS_HEALTH_CHECK) { |
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.
hmm I wonder if this would make more sense if we checked for if the health check was loading? I think we could do this in plus.slice. something like...
export const selectHealthIsLoading: (state: RootState) => boolean =
createSelector(
plusApi.endpoints.getHealth.select(),
({ isLoading }) => isLoading
);
and then also expose isLoading
as part of useFeatures
? checking for if it's loading would make more sense to me than comparing to an empty obj, but maybe it's not straightforward to implement
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.
maybe worth spending a few minutes just to see if it'd work out? otherwise we can leave it, I don't feel super strongly about it
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.
actually I take back that suggestion—I think you can just call useGetHealthQuery
directly in this component, then query for isLoading
. in fact, we do this here https://github.com/ethyca/fides/blob/main/clients/admin-ui/src/features/auth/ProtectedRoute.tsx#L41
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.
That's a good idea. It's more clear as well. I'll take a look at that.
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.
done here: 4143768
clients/admin-ui/src/features/system/add-multiple-systems/AddMultipleSystems.tsx
Show resolved
Hide resolved
Pagination :
Filter :
Otherwise looks great! 👍 |
This reverts commit 4143768.
thanks @simon-keane — I edited your comment to have checkboxes so I could keep track of which ones I knocked out. see this updated video, I think I got all your comments! Screen.Recording.2023-11-01.at.5.02.33.PM.mov |
UAT is passing! But I did create a followup bug to address the multiselect. It should select only the first page of vendors. https://ethyca.atlassian.net/browse/PROD-1306 |
Superb thanks @allisonking looks great. |
Closes PROD-1267
Description Of Changes
This PR applies a few follow items based on feedback from the original bulk vendor adding PR.
Code Changes
Steps to Confirm - UAT
TCF enabled
TCF_enabled.mov
TCF disabled
TCF_disabled.mov
Pre-Merge Checklist
CHANGELOG.md