🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level #18229

YatsukBogdan1 · 2022-10-20T12:26:22Z

What

How

This PR refactors ServiceForm and ConnectorCard components, so Formik and ServiceFormContext are moved on the top level inside ConnectorCard component. Also, I created two custom hooks to make the code inside components more readable. useConnectorCardService is responsible for logic that touches both ServiceForm and ConnectorCard. useDeleteModal is responsible for delete modal logic that is used inside EditControls and DeleteBlock components. Also, changes in this PR will be beneficial in future development since the new design Figma for ConnectorCard components uses a couple of Card components to have different form controls inside them.
Currently, the tests for the ServiceForm component is commented out due to lots of changes inside the component, planning to work on them, after approving the final state of the component.

…of control buttons

lmossman

Left some comments with my initial impressions. I like the delete changes. The useConnectorCardService though feels like it is doing a bit too much, and I'd like to see if there is any room to simplify or break that down further so that it is more readable

lmossman · 2022-10-21T00:04:45Z

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

+interface UseConnectorCardServiceHookResult {
+  advancedMode: boolean;
+  isFormSubmitting: boolean;
+  isTestConnectionInProgress: boolean;
+  job?: SynchronousJobRead | null;
+  onHandleSubmit: (values: ServiceFormValues) => Promise<void>;
+  onStopTesting: () => void;
+  saved: boolean;
+  testConnector: (v?: ServiceFormValues) => Promise<CheckConnectionRead>;
+  error: Error | null;
+  selectedConnectorDefinitionSpecificationId?: string;
+  uniqueFormId?: string;
+  jsonSchema: JSONSchema7;
+  initialValues: ServiceFormValues;
+  formFields: FormBlock;
+  validationSchema: AnySchema;
+  uiWidgetsInfo: WidgetConfigMap;
+  setUiWidgetsInfo: (widgetId: string, updatedValues: WidgetConfig) => void;
+  resetUiWidgetsInfo: () => void;
+  getValues: <T = unknown>(values: ServiceFormValues<T>) => ServiceFormValues<T>;
+  onFormSubmit: (values: ServiceFormValues) => Promise<void>;
+}


While I do think this refactor has made the ServiceForm and ConnectorCard components more readable, it feels like all of their complexity has just been shoved into this service instead.

Could you try to simplify this or break it into smaller services? It feels like there is a ton going on here which makes this service pretty hard to understand on its own without going and seeing how each result value is used.

For example, this returns an onHandleSubmit and an onFormSubmit, both of which have the same input and return types -- why are both necessary?

It feels like this service could be split up into more readable pieces. Maybe all of the uiWidget logic can be contained in a single place, and/or maybe more comments/documentation could be added here to help make it easier to understand.

yeah, that makes sense. I am currently working on this, but still a bit hard, assuming I am not familiar with logic inside this component at all

@lmossman you can check now, I added two new smaller hooks that split some logic from useConnectorCardService, and are used inside the original hook

lmossman · 2022-10-21T00:09:31Z

airbyte-webapp/src/components/DeleteBlock/useDeleteModal.ts

+
+import { DeleteBlockProps } from "./interfaces";
+
+export const useDeleteModal = ({ type, onDelete }: Partial<DeleteBlockProps>): { onDeleteButtonClick: () => void } => {


I like the introduction of this hook - its a nice way to keep this logic encapsulated while allowing it to be used in either a DeleteBlock or directly by a button

lmossman · 2022-10-21T00:10:35Z

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

-          />
-        </div>
-        {additionalSelectorComponent}
+    <Formik


Initially I thought it felt slightly weird to have the Formik be declared outside of ServiceForm, but given that the goal of this is to move the "controls" outside of the card itself, then this structure probably makes sense 👍

lmossman · 2022-10-21T00:17:40Z

airbyte-webapp/src/views/Connector/ServiceForm/components/EditControls.tsx

+              <Button type="submit" disabled={isSubmitting || !dirty || Object.keys(unfinishedFlows).length > 0}>
+                <FormattedMessage id="form.saveChangesAndTest" />
+              </Button>
+              <Button
+                className={styles.cancelButton}
+                type="button"
+                variant="secondary"
+                disabled={isSubmitting || !dirty}
+                onClick={onCancelClick}
+              >
+                <FormattedMessage id="form.cancel" />
+              </Button>


Can we swap the order of these two buttons, and change the form.saveChangesAndTest text to be Save & Test?

Those two changes would bring this in line with the design on Figma:

fixed this one

…and useUIWidget

…tton

lmossman

Had a couple more comments. I think this is looking good overall but I'm still not sure if I love the connector card hook structure.

@edmundito or @timroes could one of you take a look and let me know if you agree about the hook structure in this PR? I want to double check my thoughts with you and/or see if there are any other approaches you have in mind. Thanks!

lmossman · 2022-10-25T20:01:17Z

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

+  saved: boolean;
+}
+
+export const useOnHandeSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {


There's a typo here

Suggested change

export const useOnHandeSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {

export const useOnHandleSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {

lmossman · 2022-10-25T20:07:31Z

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

+  saved: boolean;
+}
+
+export const useOnHandeSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {


I don't know if encapsulating this logic in a hook is actually helpful, and it feels like a slightly strange thing to put in a hook to me.

I feel like it might be more readable to just put this onSubmit logic directly in the ConnectorCard component, since that is where the Formik form is defined, so having the form submission logic live close to the form itself might be nice.

If this change is made, you may even be able to get rid of the useConnectorCardService hook, and just keep the useUIWidget hook, since that one still seems useful for encapsulating the uiWidget logic.

Definitely open to feedback here though if anyone disagrees - I'm still trying to learn what the best practices are around when and how exactly hooks should be used.

dizel852 · 2022-10-27T12:39:10Z

I had a sync with @YatsukBogdan1, we were discussing the best way how to merge this PR and mine.

I noticed, that in order to render the Buttons outside of the Card, the whole card is wrapped with <Formik /> context since we need to manipulate with Form controls being in the <Formik /> context
That is exactly what we tried to avoid, and that was a goal of this PR

Few things I suggested:

move form controls outside the Card component using css - probably the worst I could suggest 😅
create two Cards: one for Dropdown, and one for Form, and tweak the styles for both, to make it look like an entire form, here is an example with hardcoded styles:

The structure will be:

<ConnectorCard>
  
  <Card>
    <ConnectorDropdownSelect>
  </Card>

  <ConnectorForm>
    <FormikContext>
        <Card>
           <Form/>
        </Card>
       
        <FormContorls>
    
    </FormikContext>
  </ConnectorForm>
</ConnectorCard>

Also, we need to keep in mind that our future design completely follows this way - splitting the Form into multiple separate Cards:
https://www.figma.com/proto/mFVUWYYjDm5zz6FFx8hTP6/FORMS-FIELDS?page-id=1%3A4&node-id=301%3A10567&viewport=606%2C64%2C0.16&scaling=min-zoom&starting-point-node-id=301%3A10567

The thing I really like, it's the ConnectorCardService hook: https://github.com/airbytehq/airbyte/pull/18229/files/a06f74a2fdd3dd7913fc1016ae7a2defd0ce27bd#r1001231672

But I'm pretty sure, the hook should be implemented after the PR will be merged, since I removed some of the redundant props due to extra calculation.

@timroes @lmossman @edmundito
Need to hear your thoughts, guys

lmossman · 2022-10-27T21:11:48Z

@dizel852 @YatsukBogdan1 @timroes My opinion here: The design that we eventually want to achieve has the connector type and name inputs in a separate cards anyway. I think the component structure you laid out above makes sense, and it also seems fine to me to just have the type & name inputs in one card, and the rest of the connector form inputs in a separate card, without doing any odd CSS things to try to make them look like a single card.

Is there any reason we shouldn't just have them in separate cards, allowing us to achieve the component structure you laid out and bringing us one step closer to the final design?

timroes · 2022-10-27T22:36:47Z

@dizel852 I would strongly advertise against CSS hacks if possible :)

Regarding the structure I also think the way your linked refactoring is going is correct and we want to have the formik context on a lower level as you did, specifically given that we've seen a lot of issue based on the formik context isn't rerendered when the type change (e.g. #18558).

I'd also second Lake in, let's try to even avoid the design hacks to move the cards together and just see it as a move towards the final design and already treat it as separate cards.

timroes · 2022-10-27T22:40:00Z

Just as an additional clarification: We're going to put this PR here on hold until we're done with #18168, right?

dizel852 · 2022-10-28T15:03:14Z

I'd also second Lake in, let's try to even avoid the design hacks to move the cards together and just see it as a move towards the final design and already treat it as separate cards.

Great! Deal!

Just as an additional clarification: We're going to put this PR here on hold until we're done with #18168, right?

Yes, I would like to do so

dizel852 · 2022-10-28T15:12:00Z

Is there any reason we shouldn't just have them in separate cards, allowing us to achieve the component structure you laid out and bringing us one step closer to the final design?

No reason 😁
But we won't be able to put ConnectorSelector(Dropdown) and Connector name fields in one separate card since the Connector name is inside the Formik context.

YatsukBogdan1 · 2022-10-31T07:31:17Z

I am moving this PR into the draft until @dizel852 PR #18168 will be merged

YatsukBogdan1 · 2022-11-07T23:22:10Z

I created a new PR for this issue #19099. If any of the current code changes will be useful later, we can still take them from this PR

edmundito · 2023-02-15T14:22:28Z

This was resolved through another approach. Closing...

YatsukBogdan1 added 5 commits October 19, 2022 23:55

Moves Formik from ServiceForm component to ConnectorCard component

8910287

Merge branch 'master' into byatusk/move-connector-card-buttons

a125887

Merge branch 'master' into byatusk/move-connector-card-buttons

7a81b8a

Refactors ServiceForm, ConnectorCard and DeleteBlock; Updates styles …

be76008

…of control buttons

Merge branch 'master' into byatusk/move-connector-card-buttons

a94a4e9

YatsukBogdan1 requested a review from a team as a code owner October 20, 2022 12:26

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Oct 20, 2022

Merge branch 'master' into byatusk/move-connector-card-buttons

a06f74a

lmossman reviewed Oct 21, 2022

View reviewed changes

YatsukBogdan1 added 6 commits October 24, 2022 15:29

Merge branch 'master' into byatusk/move-connector-card-buttons

caa1503

Merge branch 'master' into byatusk/move-connector-card-buttons

779e870

Removes redundant functions

efed94f

Splits useConnectorCardService hook into two hooks useOnHandleSubmit …

ed069d4

…and useUIWidget

Changes save button position to match figma; Updates text for save bu…

c9f0d20

…tton

Merge branch 'master' into byatusk/move-connector-card-buttons

14591eb

lmossman reviewed Oct 25, 2022

View reviewed changes

lmossman requested review from edmundito and timroes October 25, 2022 20:14

dizel852 mentioned this pull request Oct 27, 2022

Refactor <ConnectorCard /> (<ServiceForm> refactoring part 3) #18553

Closed

YatsukBogdan1 changed the title ~~🪟 🔧 Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level~~ 🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level Oct 31, 2022

YatsukBogdan1 marked this pull request as draft October 31, 2022 07:48

YatsukBogdan1 mentioned this pull request Nov 7, 2022

🪟 🔧 Refactor ConnectorForm Card component usage #19099

Closed

edmundito closed this Feb 15, 2023

edmundito deleted the byatusk/move-connector-card-buttons branch February 15, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level #18229

🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level #18229

YatsukBogdan1 commented Oct 20, 2022

lmossman left a comment

lmossman Oct 21, 2022

YatsukBogdan1 Oct 25, 2022

YatsukBogdan1 Oct 25, 2022

lmossman Oct 21, 2022

lmossman Oct 21, 2022

lmossman Oct 21, 2022

YatsukBogdan1 Oct 25, 2022

lmossman left a comment

lmossman Oct 25, 2022

lmossman Oct 25, 2022 •

edited

Loading

dizel852 commented Oct 27, 2022

lmossman commented Oct 27, 2022 •

edited

Loading

timroes commented Oct 27, 2022

timroes commented Oct 27, 2022

dizel852 commented Oct 28, 2022

dizel852 commented Oct 28, 2022

YatsukBogdan1 commented Oct 31, 2022

YatsukBogdan1 commented Nov 7, 2022

edmundito commented Feb 15, 2023


		import { DeleteBlockProps } from "./interfaces";

		export const useDeleteModal = ({ type, onDelete }: Partial<DeleteBlockProps>): { onDeleteButtonClick: () => void } => {

	export const useOnHandeSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {
	export const useOnHandleSubmit = (props: UseOnHandleSubmitHookProps): UseOnHandleSubmitHookResult => {

🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level #18229

🪟 🔧 [DRAFT] Refactor ServiceForm and ConnectorCard to move Formik and ServiceFormContext on top level #18229

Conversation

YatsukBogdan1 commented Oct 20, 2022

What

How

lmossman 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

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

dizel852 commented Oct 27, 2022

lmossman commented Oct 27, 2022 • edited Loading

timroes commented Oct 27, 2022

timroes commented Oct 27, 2022

dizel852 commented Oct 28, 2022

dizel852 commented Oct 28, 2022

YatsukBogdan1 commented Oct 31, 2022

YatsukBogdan1 commented Nov 7, 2022

edmundito commented Feb 15, 2023

lmossman Oct 25, 2022 •

edited

Loading

lmossman commented Oct 27, 2022 •

edited

Loading