🪟🔧 Move connector loading state into connector card #20127

flash1293 · 2022-12-06T13:32:35Z

What

This PR moves the loading spinner which is shown when the connection specification definition is loaded into the connector card component. This is done for a bunch of reasons:

Simplifying the connector form code - it's now only rendered if the specification is available
Remove complexity by not passing the loading prop deep into the component tree
Preparing the component to be used in other places as well

How

In the current state, the formik form is always initialized and rendered, even if there is no specification. The form component is also responsible for rendering the loading spinner itself.

This PR lifts the loading spinner to the connector card level and only renders the form component if all the prerequisites are present.

Drive-by fixes

The loading spinner requires the name of the connector that's currently loaded. This doesn't work correctly in the current state as the loading spinner pulls the name from the specification definition which isn't loaded yet. This PR changes this by passing the chosen connector id down to the connector card even if it's not loaded yet so the name can be shown in the loading spinner
In case the loading of the specification definition fails, the current state is not showing any error message because the component handling this (EditControls/CreateControls) is only showing the error if the specification definition is loaded. This PR pulls this error case to the connector card level and doesn't even attempt to render the form in this situation
There were unused props errorMessage and successMessage on the connector card level, those were removed

🚨 User Impact 🚨

Fixes a bunch of small bugs but otherwise no user-visible change.

…or-form-loading-state

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.module.scss

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx

….module.scss Co-authored-by: Tim Roes <tim@airbyte.io>

…or-form-loading-state

…irbytehq/airbyte into flash1293/connector-form-loading-state

…or-form-loading-state

krishnaglick

I haven't done a deep dive into the code but I feel like there could be an argument for adding a context to hold some of these reused and prop-drilled values. Don't hold this up on my account, merely an observation.

lmossman · 2022-12-07T22:08:16Z

airbyte-webapp/src/views/Connector/ConnectorForm/FormRoot.tsx

 import { ConnectorFormValues } from "./types";

 interface FormRootProps {
  formFields: FormBlock;
  hasSuccess?: boolean;
  isTestConnectionInProgress?: boolean;
  errorMessage?: React.ReactNode;
-  fetchingConnectorError?: Error | null;
  successMessage?: React.ReactNode;
  onRetest?: () => void;
  onStopTestingConnector?: () => void;


I can't comment on it but I think the | undefined on selectedConnector prop type (the line below this one) can now be removed, because it is now being passed in from the ConnectorForm in a branch where it is verified to not be undefined.

This should also allow us to remove the selectedConnector && () around the <CreateControls> component near the bottom of this file

Good catch, removed that stuff as well!

lmossman · 2022-12-07T22:12:26Z

airbyte-webapp/src/views/Connector/ConnectorForm/ConnectorForm.tsx

+  selectedConnectorDefinition: ConnectorDefinition;
+  selectedConnectorDefinitionSpecification: ConnectorDefinitionSpecification;


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

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.

lmossman · 2022-12-07T22:17:10Z

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx

+            </div>
+          )}
+          {fetchingConnectorError && <FetchingConnectorError />}
+          {selectedConnectorDefinition && selectedConnectorDefinitionSpecification && (


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 UX

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 and DestinationForm, the props isLoading,fetchingConnectorError and selectedConnectorDefinition are coming from a react-query hook, so one of them will always be set if there is a selection that has been made.

lmossman · 2022-12-07T22:18:57Z

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx

+          )}
+          {fetchingConnectorError && <FetchingConnectorError />}
+          {selectedConnectorDefinition && selectedConnectorDefinitionSpecification && (
+            <ConnectorForm


Also I'm getting a little confused here: your new code makes sense to me in that it only renders a <ConnectorForm> if selectedConnectorDefinition and selectedConnectorDefinitionSpecification are not nullish, but I'm confused how the old code was handling that case -- it seems like the old code was always rendering a ConnectorForm which was always rendering a FormRoot which was always rendering a FormSection, so how does the UI currently look empty when no connector is selected?

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)

lmossman · 2022-12-07T22:39:00Z

Tested it out locally and didn't notice any issues

…-form-loading-state

flash1293 · 2022-12-08T11:31:29Z

Thanks for the review @lmossman , could you check again? Removed the additional bit you found.

lmossman

Changes LGTM. Tested locally and seems to work as expected

Joe Reuter added 5 commits December 6, 2022 12:13

refactor loading state

807c436

Merge remote-tracking branch 'upstream/master' into flash1293/connect…

6c76e0f

…or-form-loading-state

move loading state up

05c3107

remove isLoading references

eee4303

remove unused props and make fetch connector error work

7d6f977

flash1293 added area/frontend team/extensibility area/frontend Related to the Airbyte webapp labels Dec 6, 2022

octavia-squidington-iv added the area/platform issues related to the platform label Dec 6, 2022

This was referenced Dec 6, 2022

🪟🔧 Remove connector form name override #20132

Merged

(Resolve) Tech debt of the ConnectorForm #14250

Closed

flash1293 marked this pull request as ready for review December 6, 2022 14:48

flash1293 requested a review from a team as a code owner December 6, 2022 14:48

Merge branch 'master' into flash1293/connector-form-loading-state

c7e7475

timroes reviewed Dec 6, 2022

View reviewed changes

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.module.scss Outdated Show resolved Hide resolved

timroes reviewed Dec 6, 2022

View reviewed changes

airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx Outdated Show resolved Hide resolved

Joe Reuter and others added 6 commits December 6, 2022 18:32

Update airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard…

9be8337

….module.scss Co-authored-by: Tim Roes <tim@airbyte.io>

Merge remote-tracking branch 'upstream/master' into flash1293/connect…

37cee8f

…or-form-loading-state

Merge branch 'flash1293/connector-form-loading-state' of github.com:a…

5686241

…irbytehq/airbyte into flash1293/connector-form-loading-state

remove undefined option for selected id

7b73364

Merge remote-tracking branch 'upstream/master' into flash1293/connect…

6d4c7d3

…or-form-loading-state

Merge remote-tracking branch 'upstream/master' into flash1293/connect…

b7a6322

…or-form-loading-state

krishnaglick reviewed Dec 7, 2022

View reviewed changes

lmossman reviewed Dec 7, 2022

View reviewed changes

Joe Reuter added 2 commits December 8, 2022 12:07

Merge remote-tracking branch 'origin/master' into flash1293/connector…

6c71028

…-form-loading-state

remove unused prop

809625f

lmossman approved these changes Dec 8, 2022

View reviewed changes

flash1293 merged commit 0e6e02f into master Dec 8, 2022

flash1293 deleted the flash1293/connector-form-loading-state branch December 8, 2022 22:28

octavia-squidington-iii mentioned this pull request Dec 9, 2022

Bump Airbyte version from 0.40.24 to 0.40.25 #20279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟🔧 Move connector loading state into connector card #20127

🪟🔧 Move connector loading state into connector card #20127

flash1293 commented Dec 6, 2022 •

edited

Loading

krishnaglick left a comment

lmossman Dec 7, 2022

flash1293 Dec 8, 2022

lmossman Dec 7, 2022

flash1293 Dec 8, 2022

lmossman Dec 7, 2022

flash1293 Dec 8, 2022

lmossman Dec 7, 2022

flash1293 Dec 8, 2022

lmossman commented Dec 7, 2022

flash1293 commented Dec 8, 2022

lmossman left a comment

		selectedConnectorDefinition: ConnectorDefinition;
		selectedConnectorDefinitionSpecification: ConnectorDefinitionSpecification;

🪟🔧 Move connector loading state into connector card #20127

🪟🔧 Move connector loading state into connector card #20127

Conversation

flash1293 commented Dec 6, 2022 • edited Loading

What

How

Drive-by fixes

🚨 User Impact 🚨

krishnaglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman commented Dec 7, 2022

flash1293 commented Dec 8, 2022

lmossman left a comment

Choose a reason for hiding this comment

flash1293 commented Dec 6, 2022 •

edited

Loading