-
Notifications
You must be signed in to change notification settings - Fork 78
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
New integration options for D&D #5000
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -14,7 +14,7 @@ import { | |||
import type { RootState } from "../../app/store"; | |||
import { | |||
ConnectionTypeParams, | |||
ConnectionTypeSecretSchemaReponse, | |||
ConnectionTypeSecretSchemaResponse, |
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.
This was just a typo that got preserved from way back when.
const onChangeFilter = (newIndex: number) => { | ||
setIsFiltering(true); | ||
setTabIndex(newIndex); | ||
setTimeout(() => setIsFiltering(false), 100); |
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.
Putting in a fake "loading" state for a short period makes the change in contents feel less jarring to me-- not married to this if we're against 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.
I think small artificial delays are valid if it improves the UX. We can keep it.
Passing run #8413 ↗︎
Details:
Review all test suite changes for PR #5000 ↗︎ |
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.
Overall the code looks very good! Great work. Just a question about the missing copies for some integrations.
And two UI bugs:
- This cancel button seems to be much bigger than all of the other ones, I think it should be the same size

- The active tab highlight. I think there may be a separate issue for this, but if there's a quick fix for it, it would be nice to add it. It's very explicit because when you open the modal the first tab already has the highlight rectangle.

@@ -0,0 +1,9 @@ | |||
import { Flex, Spinner, SpinnerProps } from "fidesui"; | |||
|
|||
const FidesSpinner = ({ ...props }: SpinnerProps) => ( |
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 one making this reusable
import { Collapse, Text, useDisclosure } from "fidesui"; | ||
import { ReactNode } from "react"; | ||
|
||
const ShowMoreContent = ({ children }: { children: ReactNode }) => { |
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.
yes, reusability!
@@ -1,7 +1,8 @@ | |||
import { UseDisclosureReturn } from "fidesui"; | |||
|
|||
import AddModal from "~/features/configure-consent/AddModal"; | |||
import ConfigureIntegrationForm from "~/features/integrations/ConfigureIntegrationForm"; | |||
import ConfigureIntegrationForm from "~/features/integrations/add-integration/ConfigureIntegrationForm"; | |||
import useIntegrationOption from "~/features/integrations/useIntegrationOption"; | |||
import { ConnectionConfigurationResponse } from "~/types/api"; | |||
|
|||
const ConfigureIntegrationModal = ({ |
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.
composition for the win!
<> | ||
<InfoHeading text="Overview" /> | ||
<InfoText> | ||
Here's some example copy talking about Amazon S3. Lorem ipsum dolor |
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.
Are the real copies ready?
<> | ||
<InfoHeading text="Overview" /> | ||
<InfoText> | ||
Here's some example copy talking about DynamoDB. Lorem ipsum dolor |
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.
Do we have the final copy?
const onChangeFilter = (newIndex: number) => { | ||
setIsFiltering(true); | ||
setTabIndex(newIndex); | ||
setTimeout(() => setIsFiltering(false), 100); |
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 think small artificial delays are valid if it improves the UX. We can keep 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.
Thanks for the button fix. approve
Passing run #8417 ↗︎
Details:
Review all test suite changes for PR #5000 ↗︎ |
Closes PROD-2155
Description Of Changes
Adds UI to configure integrations and add discovery monitors for S3, Scylla, and DynamoDB to admin UI.
(Note these screenshots don't show the correct logo for S3, but I've fixed that!)
Code Changes
Steps to Confirm
In new integrations UI:
Pre-Merge Checklist
CHANGELOG.md