[Connector Builder] Configuration UI MVP #20008

lmossman · 2022-12-01T23:22:19Z

What

This PR adds the first iteration of the Configuration UI to the Connector Builder, allowing users to fill out UI forms rather than hand-writing a YAML file.

I have recorded a loom where I go through what it looks like to build a simple connector in the new UI, and the various other features that have been added:
https://www.loom.com/share/9cf4ddb8ad894503bbc82cda5606aa2b

The main thing left to address for this first iteration is to better handle error cases, e.g. if a required field is not filled out and the user tries to test, currently they will see a somewhat confusing error message. This will be addressed in a later PR.

How

This PR is doing quite a bit -- apologies for such a large changeset. The motivation behind putting all of these changes in one PR was that I didn't want to add the new UI view until it reached at least minimum usability for building a simple connector like the Poke API.

However, I could have still created incremental PRs into this feature branch to get review as I went, so I will aim to do things that way in the future.

🚨 User Impact 🚨

Users can now configure connectors through UI forms in the connector builder. More functionality to come soon!

…nectorBuilder directory

…ectly

lmossman · 2022-12-06T23:49:14Z

airbyte-webapp/src/components/ui/ResizablePanels/ResizablePanels.tsx

        propagateDimensions
        minSize={firstPanel.minWidth}
        flex={firstPanel.flex}
        onStopResize={(args) => {
          firstPanel.onStopResize?.(args.component.props.flex);
        }}
      >
-        <PanelContainer className={firstPanel.className} overlay={firstPanel.overlay}>


Applying the styling to this container wasn't really doing much due to its nesting. What we really wanted users of this component to control was the ReflexElement for that panel, as that is the parent container of it.

This allowed me to fix the rounded corners of the testing side panel in the connector builder as well as the box shadow of it.

lmossman · 2022-12-08T23:20:31Z

airbyte-webapp/src/services/connectorBuilder/connector_manifest_openapi.yaml

+    ConnectorManifest:
+      $ref: "../../../../airbyte-cdk/python/airbyte_cdk/sources/declarative/config_component_schema.json"


FYI I added this file so that we could use orval to pull in the Connector Manifest json schema and generate typescript types from it. This is necessary to do rather than referencing the schema directly in the connector builder server openapi spec, because of the issue described here: #20248

lmossman · 2022-12-08T23:26:03Z

Do we want to allow to test the connector if base url is not defined / invalid in some way we know?

No we do not -- this is what I am currently working on fixing and will be addressing in a follow-up PR. Basically, I will be making disabling the test button if they click it and there are any errors in the formik form, and will work on making it clear which views have errors that need to be fixed.

Weird idea - I found myself trying to do command+enter to re-test, what do you think?

Interesting, I hadn't considered hotkeys for testing, but I think that could definitely be useful since re-testing is pretty common. Created an issue for that here: https://github.com/airbytehq/airbyte-internal-issues/issues/1184

I think with that I have addressed all of your comments. Could you take another look @flash1293

flash1293

I focused mostly on small css stuff. In general I think we should invest in general layout components that make it easier to achieve common layouts to make these things much easier, but I will bring that up separately with the frontend chapter.

Besides that and a few nits it starts to look pretty good.

Phrase is not translated in modal:

A more general one - while testing I noticed the UI sends a request for every keystroke which seems like a lot for production systems:

Can we throttle these calls?

flash1293 · 2022-12-09T14:03:05Z

airbyte-webapp/src/components/connectorBuilder/Builder/AddStreamButton.module.scss

+  gap: variables.$spacing-xl;
+}
+
+.addButton {


I think it's OK for now, but I don't like how much you need to fight the stylings of the underlying button component.

It seems like it would be possible to make the round icon button a prop on the regular button - this way you don't need to overrule styles with !important but you can selectively apply the right styles in the right moment.

If you think that makes sense I can create an issue for it.

Yeah I think that makes sense - having a prop on the button to easily make it round would be really nice here. If you could create an issue for that, that would be great!

flash1293 · 2022-12-09T14:04:10Z

airbyte-webapp/src/components/connectorBuilder/Builder/BuilderSidebar.tsx

+        </Text>
+
+        <AddStreamButton
+          className={styles.addStreamButton}


You are adding some styles here and then there are additional styles in the AddStreamButton component itself. Can we consolidate these in a single place?

This is a pattern I've been doing for a little while, but let me know if you think I shouldn't be doing it:

When I create a custom component, I've usually been trying to keep positional styling (e.g. the addStreamButton style in BuilderSidebar.module.scss which just sets margin values) in the same .module.scss file as the parent component that holds the custom component, because that feels like the most natural place to control the position.

I then put the styling code for the internals of the custom component (e.g. the addButton style in the AddStreamButton.module.scss file) in the custom component's own scss file, since that feels like the most natural place to style internals of a component.

Basically it felt weird to me to put positional styling inside the custom component's own styles, because if I want to use that custom component in another place in the future then I would need to lift those positional styles back up to the parent anyway. I guess that doesn't really apply so much here because the "Add stream" button isn't likely to be reused somewhere else, so I'll consolidate these styles into the AddStreamButton component's styles now.

But what do you think of that pattern overall?

I was actually able to solve this specific case in I think a much more idiomatic way, see commit here: b9580b4

I just used the flex property on the heading to make it grow to fill the rest of the flexbox, so I didn't need to apply the margin styles to the AddStreamButton anymore to make it positioned correctly. So maybe when I find myself wanting to do the above pattern, I should first ask myself if I can solve it in a more idiomatic way like this (though I'm sure not every case will end up like this)

But what do you think of that pattern overall?

Thanks for the explanation, that makes a lot of sense to me as a pattern - Ideally I hope we can abstract away most of these concerns into library components, but I'll bring it up in the chapter sync.

airbyte-webapp/src/components/connectorBuilder/Builder/BuilderSidebar.module.scss

airbyte-webapp/src/components/connectorBuilder/YamlEditor/YamlEditor.module.scss

airbyte-webapp/src/components/connectorBuilder/StreamTestingPanel/StreamSelector.module.scss

airbyte-webapp/src/pages/ConnectorBuilderPage/ConnectorBuilderPage.module.scss

flash1293 · 2022-12-09T14:46:11Z

airbyte-webapp/src/components/connectorBuilder/Builder/Builder.tsx

+  const handleViewSelect = (selectedView: BuilderView, streamName?: string) => {
+    setSelectedView(selectedView);
+    if (selectedView !== "global" && streamName !== undefined) {
+      setSelectedStream(streamName);


We are auto-selecting the testing panel for the stream when switching the view, but we don't do it the other way around (switching the view when switching the selectedStream). Would it make sense to do so?

Related to that - would it make sense to lift the selected view into the ConnectorBuilderStateService as well? It's conceptually very similar to the selected stream, it confused me at first why it's handled in such a different fashion.

Not a must-have though.

Good suggestion -- I've added this behavior. The one slightly nuanced aspect of it is that if the Global Configuration view is currently selected while the user changes streams in the testing panel dropdown, I don't think we want it to switch the view to that stream, because the user might want to test out different streams while iterating on global configuration values. So I've encoded that behavior as well.

Makes a lot of sense to me!

lmossman · 2022-12-09T23:17:02Z

Can we throttle these calls?

I've pushed this commit to debounce setting the json manifest from the yaml editor, which seems to solve that problem: 5ffe20e

…cted stream on rename

lmossman · 2022-12-10T01:59:10Z

@flash1293 I made a couple more changes in the last few commits, which do the following:

If a stream's name is set to an empty/pure whitespace string, it now shows an (empty) placeholder in the view select button and test panel stream dropdown
I switched the tracking of the selected test panel stream to track the index instead of the stream name, as this was necessary to ensure that if a user changes the name of a stream, that stream stays selected in the test panel

I believe I have also addressed all of the other feedback so this should be ready for another look

flash1293

For the scope of this PR it LGTM, great work!

Two notes that should be addressed, can happen later on:

The "one request per keystroke" also happens for the test input - this hasn't been touched here, so we can address it separately, just something I noticed. Not sure whether we even need to re-fetch the streams in this situation, but I might miss something
The "add stream" modal doesn't really add any value, it's just making the user flow more complex - it seems like it would be easier to add an empty, untouched stream and switch to the view so the user can fill it in without any modal involved. I realize that this is not according to the current designs so happy to discuss it in some forum before implementing. Let me know if I should create an issue for it.

lmossman · 2022-12-12T19:05:01Z

The "one request per keystroke" also happens for the test input - this hasn't been touched here, so we can address it separately, just something I noticed. Not sure whether we even need to re-fetch the streams in this situation, but I might miss something

One reason to refetch the streams in this situation is if the user has implemented a stream in which the name or the URL use some interpolated user-input value. Then the name/url of that stream depend on the value they type into the test input menu, so they need to be refetched to resolve those interpolated references.

I won't address it in this PR but maybe that is something you can improve in your PR to use the Connector Form for the test input menu?

The "add stream" modal doesn't really add any value, it's just making the user flow more complex - it seems like it would be easier to add an empty, untouched stream and switch to the view so the user can fill it in without any modal involved. I realize that this is not according to the current designs so happy to discuss it in some forum before implementing. Let me know if I should create an issue for it.

I think the benefits of using a modal are that simplifies the set of fields that the user is presented with upon creating a new stream to only the ones that are absolutely required (name and path), and doesn't allow them to create a stream unless they supply those values. If we just create an empty stream and focus that view, it may be a bit more overwhelming (especially once we add more components to the stream view). Though that may be lessened if we have collapsible components.

@flash1293 could you create a ticket to capture this question? We can discuss it in the next low-code builder sync

flash1293 · 2022-12-12T19:15:03Z

One reason to refetch the streams in this situation is if the user has implemented a stream in which the name or the URL use some interpolated user-input value. Then the name/url of that stream depend on the value they type into the test input menu, so they need to be refetched to resolve those interpolated references.

Makes sense, thanks

I won't address it in this PR but maybe that is something you can improve in your PR to use the Connector Form for the test input menu?

Sounds good, I'm gonna take care of it.

I think the benefits of using a modal are that simplifies the set of fields that the user is presented with upon creating a new stream to only the ones that are absolutely required (name and path), and doesn't allow them to create a stream unless they supply those values. If we just create an empty stream and focus that view, it may be a bit more overwhelming (especially once we add more components to the stream view). Though that may be lessened if we have collapsible components.

I see your point, I still think we can shape the general form in a way to make it more approachable without resorting to modals.

flash1293 · 2022-12-12T19:22:42Z

I see your point, I still think we can shape the general form in a way to make it more approachable without resorting to modals.

Added it to the next sync

lmossman added 2 commits December 1, 2022 15:01

move connector builder components into the same shared components/con…

699470e

…nectorBuilder directory

move diff over from poc branch

827532b

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 1, 2022

Base automatically changed from lmossman/connector-builder-component-folder to master December 2, 2022 18:29

lmossman added 8 commits December 2, 2022 16:04

save current progress

4899673

add modal for adding streams

a1b62f1

focus stream after adding and reset button style

39e8ba8

add reset confirm modal and select view on add

2352ef9

style global config and streams buttons

df492fa

styling improvements

5a99c7a

handle long stream names better

62fc08c

pull in connector manifest schema directly

7b26b78

octavia-squidington-iv added the CDK Connector Development Kit label Dec 6, 2022

lmossman temporarily deployed to more-secrets December 6, 2022 02:53 Inactive

add box shadows to resizable panels

71a88f1

lmossman temporarily deployed to more-secrets December 6, 2022 19:56 Inactive

lmossman added 2 commits December 6, 2022 14:54

upgrade orval and use connector manifest schema directly

d7bee54

remove airbyte protocol from connector builder api spec

162ecf2

lmossman changed the base branch from master to lmossman/use-connector-manifest-schema-directly December 6, 2022 23:11

lmossman added 3 commits December 6, 2022 15:13

Merge branch 'master' into lmossman/use-connector-manifest-schema-dir…

def60b0

…ectly

generate python models from openapi change

ff9aa76

merge with lmossman/use-connector-manifest-schema-directly

3678956

lmossman temporarily deployed to more-secrets December 6, 2022 23:38 — with GitHub Actions Inactive

lmossman commented Dec 6, 2022

View reviewed changes

lmossman added 2 commits December 6, 2022 15:57

fix position of yaml toggle

b16d4a8

handle no stream case with better looking message

67fb9dd

Base automatically changed from lmossman/use-connector-manifest-schema-directly to master December 7, 2022 01:29

group global fields into single object and fix console error

bb42007

lmossman commented Dec 8, 2022

View reviewed changes

flash1293 reviewed Dec 9, 2022

View reviewed changes

lmossman added 2 commits December 9, 2022 14:43

use correct intl string id

d6f10b4

throttle setting json manifest in yaml editor

5ffe20e

lmossman added 13 commits December 9, 2022 15:21

use button prop instead of manually styling

b82ba2a

consolidate AddStreamButton styles

b9580b4

fix sidebar flex styles

1587243

use specific flex properties instead of flex

62a60e5

clean up download and reset button styles

092821d

use row-reverse for yaml editor download button

7b8dec8

fix stream selector styles to remove margins

6d68db9

give connector setup guide panel same corner and shadow styles

91bc357

remove blur from page display

4ffb969

set view to stream when selected in test panel

70afdf8

add placeholder when stream name is empty

f656506

switch to index-based stream selection to preserve testing panel sele…

c437d30

…cted stream on rename

handle empty name in stream selector

3cdbc70

flash1293 approved these changes Dec 12, 2022

View reviewed changes

merge with master

b4e96ec

lmossman merged commit 7afc689 into master Dec 12, 2022

lmossman deleted the lmossman/connector-builder-config-ui branch December 12, 2022 21:16

github-actions bot mentioned this pull request Dec 13, 2022

Bump helm chart version reference to 0.43.3 #20408

Merged

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

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connector Builder] Configuration UI MVP #20008

[Connector Builder] Configuration UI MVP #20008

lmossman commented Dec 1, 2022 •

edited

Loading

lmossman Dec 6, 2022

lmossman Dec 8, 2022

lmossman commented Dec 8, 2022

flash1293 left a comment

flash1293 Dec 9, 2022

lmossman Dec 9, 2022

flash1293 Dec 9, 2022

lmossman Dec 9, 2022 •

edited

Loading

lmossman Dec 9, 2022

flash1293 Dec 12, 2022

flash1293 Dec 9, 2022

lmossman Dec 10, 2022

flash1293 Dec 12, 2022

lmossman commented Dec 9, 2022

lmossman commented Dec 10, 2022 •

edited

Loading

flash1293 left a comment •

edited

Loading

lmossman commented Dec 12, 2022

flash1293 commented Dec 12, 2022

flash1293 commented Dec 12, 2022

		ConnectorManifest:
		$ref: "../../../../airbyte-cdk/python/airbyte_cdk/sources/declarative/config_component_schema.json"

[Connector Builder] Configuration UI MVP #20008

[Connector Builder] Configuration UI MVP #20008

Conversation

lmossman commented Dec 1, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman commented Dec 8, 2022

flash1293 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

lmossman Dec 9, 2022 • edited Loading

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 9, 2022

lmossman commented Dec 10, 2022 • edited Loading

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

lmossman commented Dec 12, 2022

flash1293 commented Dec 12, 2022

flash1293 commented Dec 12, 2022

lmossman commented Dec 1, 2022 •

edited

Loading

lmossman Dec 9, 2022 •

edited

Loading

lmossman commented Dec 10, 2022 •

edited

Loading

flash1293 left a comment •

edited

Loading