-
Notifications
You must be signed in to change notification settings - Fork 77
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
Upgrade Privacy Request table to use FidesTable V2 #4990
Upgrade Privacy Request table to use FidesTable V2 #4990
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
1aa3660
to
aebafff
Compare
Passing run #8356 ↗︎
Details:
Review all test suite changes for PR #4990 ↗︎ |
Everything working here except for filters
5e7282b
to
04da160
Compare
@@ -74,19 +75,20 @@ const tableHeaderButtonStyles = { | |||
}, | |||
}; | |||
|
|||
interface HeaderContentProps<T> { |
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.
Question for you, is there a significant difference between TS interfaces and types for component props?
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.
Great question.
Yes and No. The general rule is "use interface
until you need type
". Not a very big difference, especially if you aren't exporting it. If exporting, you run the risk of 2 interfaces with the same name conflicting (extending) one another accidentally, where that's not possible with type.
For example:
interface Props {
prop1: boolean;
}
interface Props {
prop2: string;
}
creates the equivalent of
interface Props {
prop1: boolean;
prop2: string;
}
if those were both using type
then only prop2
would apply.
I think the main reason our app is inundated with type
is because there is no naming convention and just plain old Props
is being used everywhere, which was likely creating the scenario where they were accidentally extending each other.
I prefer interface
for components because a) if you are consistent with naming conventions (eg. ${componentName}Props
) and never export them unless absolutely necessary (rare) then the general rule "use interface
until you need type
" applies. b) this is what most component libraries are using (see Chakra's components, eg. BoxProps
) so extending those with like definitions feels cleaner. c) nitpicky: it's more clear to use extends
than &
IMO.
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.
Passing run #8357 ↗︎
Details:
Review all test suite changes for PR #4990 ↗︎ |
Closes PROD-1786
Description Of Changes
Update the privacy request screen to utilize the new table
Code Changes
MultiSelectTag
component for filtering by multiple values per columnSteps to Confirm
/privacy-requests
Pre-Merge Checklist
CHANGELOG.md