-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟🔧 Move connector loading state into connector card #20127
Changes from 12 commits
807c436
6c76e0f
05c3107
eee4303
7d6f977
c7e7475
9be8337
37cee8f
5686241
7b73364
6d4c7d3
b7a6322
6c71028
809625f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { FormattedMessage } from "react-intl"; | |
|
||
import { JobItem } from "components/JobItem/JobItem"; | ||
import { Card } from "components/ui/Card"; | ||
import { Spinner } from "components/ui/Spinner"; | ||
|
||
import { | ||
Connector, | ||
|
@@ -19,6 +20,8 @@ import { ConnectorCardValues, ConnectorForm, ConnectorFormValues } from "views/C | |
|
||
import { useDocumentationPanelContext } from "../ConnectorDocumentationLayout/DocumentationPanelContext"; | ||
import { ConnectorDefinitionTypeControl } from "../ConnectorForm/components/Controls/ConnectorServiceTypeControl"; | ||
import { FetchingConnectorError } from "../ConnectorForm/components/TestingConnectionError"; | ||
import ShowLoadingMessage from "./components/ShowLoadingMessage"; | ||
import styles from "./ConnectorCard.module.scss"; | ||
import { useAnalyticsTrackFunctions } from "./useAnalyticsTrackFunctions"; | ||
import { useTestConnector } from "./useTestConnector"; | ||
|
@@ -38,14 +41,16 @@ interface ConnectorCardBaseProps { | |
|
||
// used in ConnectorCard and ConnectorForm | ||
formType: "source" | "destination"; | ||
/** | ||
* id of the selected connector definition id - might be available even if the specification is not loaded yet | ||
* */ | ||
selectedConnectorDefinitionId: string | null; | ||
selectedConnectorDefinitionSpecification?: ConnectorDefinitionSpecification; | ||
isEditMode?: boolean; | ||
|
||
// used in ConnectorForm | ||
formId?: string; | ||
fetchingConnectorError?: Error | null; | ||
errorMessage?: React.ReactNode; | ||
successMessage?: React.ReactNode; | ||
hasSuccess?: boolean; | ||
isLoading?: boolean; | ||
} | ||
|
@@ -70,6 +75,8 @@ export const ConnectorCard: React.FC<ConnectorCardCreateProps | ConnectorCardEdi | |
jobInfo, | ||
onSubmit, | ||
additionalSelectorComponent, | ||
selectedConnectorDefinitionId, | ||
fetchingConnectorError, | ||
...props | ||
}) => { | ||
const [saved, setSaved] = useState(false); | ||
|
@@ -96,7 +103,8 @@ export const ConnectorCard: React.FC<ConnectorCardCreateProps | ConnectorCardEdi | |
} = props; | ||
|
||
const selectedConnectorDefinitionSpecificationId = | ||
selectedConnectorDefinitionSpecification && ConnectorSpecification.id(selectedConnectorDefinitionSpecification); | ||
selectedConnectorDefinitionId || | ||
(selectedConnectorDefinitionSpecification && ConnectorSpecification.id(selectedConnectorDefinitionSpecification)); | ||
|
||
const selectedConnectorDefinition = useMemo( | ||
() => availableConnectorDefinitions.find((s) => Connector.id(s) === selectedConnectorDefinitionSpecificationId), | ||
|
@@ -175,25 +183,34 @@ export const ConnectorCard: React.FC<ConnectorCardCreateProps | ConnectorCardEdi | |
</div> | ||
{additionalSelectorComponent} | ||
<div> | ||
<ConnectorForm | ||
// Causes the whole ConnectorForm to be unmounted and a new instance mounted whenever the connector type changes. | ||
// That way we carry less state around inside it, preventing any state from one connector type from affecting another | ||
// connector type's form in any way. | ||
key={selectedConnectorDefinition && Connector.id(selectedConnectorDefinition)} | ||
{...props} | ||
selectedConnectorDefinition={selectedConnectorDefinition} | ||
selectedConnectorDefinitionSpecification={selectedConnectorDefinitionSpecification} | ||
isTestConnectionInProgress={isTestConnectionInProgress} | ||
onStopTesting={onStopTesting} | ||
testConnector={testConnector} | ||
onSubmit={onHandleSubmit} | ||
formValues={formValues} | ||
errorMessage={props.errorMessage || (error && generateMessageFromError(error))} | ||
successMessage={ | ||
props.successMessage || (saved && props.isEditMode && <FormattedMessage id="form.changesSaved" />) | ||
} | ||
connectorId={isEditMode ? getConnectorId(props.connector) : undefined} | ||
/> | ||
{props.isLoading && ( | ||
<div className={styles.loaderContainer}> | ||
<Spinner /> | ||
<div className={styles.loadingMessage}> | ||
<ShowLoadingMessage connector={selectedConnectorDefinition?.name} /> | ||
</div> | ||
</div> | ||
)} | ||
{fetchingConnectorError && <FetchingConnectorError />} | ||
{selectedConnectorDefinition && selectedConnectorDefinitionSpecification && ( | ||
<ConnectorForm | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I'm getting a little confused here: your new code makes sense to me in that it only renders a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, it would render all of the components, but if you follow all the branches in the tree they all used to end in an empty component eventually if nothing is selected (e.g. the form in this case only contains an empty form section which renders an empty fragment) |
||
// Causes the whole ConnectorForm to be unmounted and a new instance mounted whenever the connector type changes. | ||
// That way we carry less state around inside it, preventing any state from one connector type from affecting another | ||
// connector type's form in any way. | ||
key={selectedConnectorDefinition && Connector.id(selectedConnectorDefinition)} | ||
{...props} | ||
selectedConnectorDefinition={selectedConnectorDefinition} | ||
selectedConnectorDefinitionSpecification={selectedConnectorDefinitionSpecification} | ||
isTestConnectionInProgress={isTestConnectionInProgress} | ||
onStopTesting={onStopTesting} | ||
testConnector={testConnector} | ||
onSubmit={onHandleSubmit} | ||
formValues={formValues} | ||
errorMessage={error && generateMessageFromError(error)} | ||
successMessage={saved && props.isEditMode && <FormattedMessage id="form.changesSaved" />} | ||
connectorId={isEditMode ? getConnectorId(props.connector) : undefined} | ||
/> | ||
)} | ||
{/* Show the job log only if advanced mode is turned on or the actual job failed (not the check inside the job) */} | ||
{job && (advancedMode || !job.succeeded) && <JobItem job={job} />} | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,14 +87,12 @@ const RevalidateOnValidationSchemaChange: React.FC<{ validationSchema: unknown } | |
export interface ConnectorFormProps { | ||
formType: "source" | "destination"; | ||
formId?: string; | ||
selectedConnectorDefinition?: ConnectorDefinition; | ||
selectedConnectorDefinitionSpecification?: ConnectorDefinitionSpecification; | ||
selectedConnectorDefinition: ConnectorDefinition; | ||
selectedConnectorDefinitionSpecification: ConnectorDefinitionSpecification; | ||
Comment on lines
+90
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A side note that doesn't need to be addressed in this PR: when we eventually want to use the component in the Connector Builder (#17674), there won't be any concept of "connector definitions" to pass to that component. Instead we will just have a JSON schema object that we need to pass in and get back a rendered connector form (or something similar). So it would be great if this component could be generified in that way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'm not sure yet at which level we can lift out the functionality required for the connector builder, I guess we need to move a few things around. I will go through with the rest of the cleanup and take a look then. |
||
onSubmit: (values: ConnectorFormValues) => Promise<void>; | ||
isLoading?: boolean; | ||
isEditMode?: boolean; | ||
formValues?: Partial<ConnectorFormValues>; | ||
hasSuccess?: boolean; | ||
fetchingConnectorError?: Error | null; | ||
errorMessage?: React.ReactNode; | ||
successMessage?: React.ReactNode; | ||
connectorId?: string; | ||
|
@@ -112,7 +110,6 @@ export const ConnectorForm: React.FC<ConnectorFormProps> = (props) => { | |
formType, | ||
formValues, | ||
onSubmit, | ||
isLoading, | ||
isEditMode, | ||
isTestConnectionInProgress, | ||
onStopTesting, | ||
|
@@ -132,13 +129,13 @@ export const ConnectorForm: React.FC<ConnectorFormProps> = (props) => { | |
...(selectedConnectorDefinitionSpecification ? { name: { type: "string" } } : {}), | ||
...Object.fromEntries( | ||
Object.entries({ | ||
connectionConfiguration: isLoading ? null : specifications, | ||
connectionConfiguration: specifications, | ||
}).filter(([, v]) => !!v) | ||
), | ||
}, | ||
required: ["name"], | ||
}), | ||
[isLoading, selectedConnectorDefinitionSpecification, specifications] | ||
[selectedConnectorDefinitionSpecification, specifications] | ||
); | ||
|
||
const { formFields, initialValues } = useBuildForm(jsonSchema, formValues); | ||
|
@@ -199,7 +196,6 @@ export const ConnectorForm: React.FC<ConnectorFormProps> = (props) => { | |
selectedConnectorDefinition={selectedConnectorDefinition} | ||
selectedConnectorDefinitionSpecification={selectedConnectorDefinitionSpecification} | ||
isEditMode={isEditMode} | ||
isLoadingSchema={isLoading} | ||
validationSchema={validationSchema} | ||
connectorId={connectorId} | ||
> | ||
|
This file was deleted.
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.
Is it possible that one of these is nullish even though a user has clicked on a connector from the dropdown? If so we may want to show some sort of error message here indicating that the connector could not be loaded or something. Because based on this code it still seems possible for
fetchingConnectorError
to be nullish, and for one of these values to be nullish as well, in which case we just won't render anything here which feels like a not great UXThere 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 don't think there is a way to run into this situation besides the desired one when the user navigates to a completely empty form.
If you look at the code where the component is used in
SourceForm
andDestinationForm
, the propsisLoading
,fetchingConnectorError
andselectedConnectorDefinition
are coming from a react-query hook, so one of them will always be set if there is a selection that has been made.