-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
🪟 🔧 Refactored <ServiceForm /> #17597
Conversation
# Conflicts: # airbyte-webapp/src/views/Connector/ServiceForm/components/TestingConnectionSpinner.module.scss # airbyte-webapp/src/views/Connector/ServiceForm/components/TestingConnectionSpinner.tsx
- update tests - updates storybook
- update tests - updates storybook
- move utility functions outside of component - move analytics tracking functions to separate component hook
- update tests - remove test with checking serviceType
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 really like where this is going. Lots of good changes there. I put comments on areas that could be improved.
In the future, I would have liked to see this change split into separate PRs that change one aspect of the code. The service type change, the analytics changes, and scss refactor done at the same time means that we will need to spend more time testing this before merge to make sure there were no issues.
...-webapp/src/pages/DestinationPage/pages/CreateDestinationPage/components/DestinationForm.tsx
Outdated
Show resolved
Hide resolved
> | ||
{({ setFieldValue }) => ( | ||
<Form> | ||
<ModalBody className={styles.modalBody}> |
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.
ModalBody should be outside of the form/formik. Instead it should be
<ModalBody>
<Formik>
<Form>
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.
Agree. Fixed. But needed to add additional submit handler since the "submit" button is in the ModalFooter (outside the Formik context)
...-webapp/src/pages/DestinationPage/pages/CreateDestinationPage/components/DestinationForm.tsx
Outdated
Show resolved
Hide resolved
<ConnectorCard | ||
icon={icon} | ||
releaseStage={releaseStage} | ||
connectionName={formatMessage({ id: "destinations.dontHaveYourOwnDestination" })} | ||
connectorName={formatMessage({ id: "destinations.startWith" }, { name })} | ||
fullWidth | ||
/> |
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 wonder if it makes sense to put this back on StartWithDestination
and render children
here. This component seems good for isolating the card style but looks like the logic is still up in StartWithDestination
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'm not sure I'm quite understanding what the value of this component is. Why is it worse to just have all of this code in StartWithDestination
, and all of the styles that are associated with that component in the same SCSS module file?
airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx
Outdated
Show resolved
Hide resolved
...s/Connector/ServiceForm/components/Controls/ConnectorServiceTypeControl/utilityFunctions.tsx
Outdated
Show resolved
Hide resolved
@@ -97,6 +87,7 @@ const DestinationStep: React.FC<Props> = ({ onNextStep, onSuccess }) => { | |||
errorMessage={errorMessage} | |||
selectedConnectorDefinitionSpecification={destinationDefinitionSpecification} | |||
isLoading={isLoading} | |||
formValues={destinationDefinitionId ? { serviceType: destinationDefinitionId } : undefined} |
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.
Would it make more sense to set this prop as serviceType
or connectorDefinitionId
? and then create the object inside the ConnectorCard
?
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.
{additionalDropdownComponent} | ||
</div> | ||
</Card> | ||
{intermediateComponent} |
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 could use a better name as well. It's unclear what intermediateComponent
means without having to look at the implementation
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.
additionalBottomComponent
- sounds better?
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.
Since this intermediateComponent
prop is only used in one place (DestinationForm), and it is just being placed after everything else in the ConnectorCard component, would it maybe make more sense to just remove the prop-drilling and place the StartWithDestination
component after the ConnectorCard
component in DestinationForm
?
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.
Hmm... Yep, you are right - in this case, it's definitely better to move it to the bottom and remove the prop-drilling. 👍
Since we had a talk with Nico and decided not to split the whole ConnectorCard
into multiple Card
components (leaving the old design) at least for now.
So I will remove the intermediateComponent
.
PS.
The reason why I called it intermediateComponent
is because of the design: https://www.figma.com/file/8aM4gBoW95whyshI5kwVct/01_06_SOURCES?node-id=405%3A4015
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.
@dizel852 cool, once that change is made then I think this PR looks good to me from my end!
airbyte-webapp/src/views/Connector/ConnectorCard/useAnalyticsTrackFunctions.tsx
Outdated
Show resolved
Hide resolved
}, | ||
/* since we use <ConnectorServiceTypeControl/> outside formik form | ||
we need to keep the serviceType field in formik, but hide it */ | ||
component: () => null, |
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 need to define a null
component?
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.
Replied below
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.
Left some comments. Overall I think these changes look good and bring the ServiceForm closer in line to what we want, so nice job!
There are still some comments from Edmundo and merge conflicts that should be addressed before approval. I agree with his sentiment that these changes would have been easier to test and review if they had been made incrementally in smaller PRs
/* since we use <ConnectorServiceTypeControl/> outside formik form | ||
we need to keep the serviceType field in formik, but hide 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'm not quite understanding why we still need to keep the serviceType
field here. Why can't it be removed?
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 the idea is that it's automatically part of the values, that will be send to the API later. If that's the case, I'd also still like to see this removed. I think we should just once when sending to the API actually merge the serviceType from the other select into the request, and not have this at all part of Formik.
@@ -57,15 +42,15 @@ const FormRoot: React.FC<FormRootProps> = ({ | |||
const { resetServiceForm, isLoadingSchema, selectedService, isEditMode, formType } = useServiceForm(); | |||
|
|||
return ( | |||
<FormContainer> | |||
<Form className={classnames(styles.formContainer, { [styles.emptyContainer]: !selectedConnector })}> |
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.
Option 3 sounds the best to me out of the ones you listed
<ConnectorCard | ||
icon={icon} | ||
releaseStage={releaseStage} | ||
connectionName={formatMessage({ id: "destinations.dontHaveYourOwnDestination" })} | ||
connectorName={formatMessage({ id: "destinations.startWith" }, { name })} | ||
fullWidth | ||
/> |
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'm not sure I'm quite understanding what the value of this component is. Why is it worse to just have all of this code in StartWithDestination
, and all of the styles that are associated with that component in the same SCSS module file?
> | ||
{({ setFieldValue }) => ( | ||
<Form> | ||
<ModalBody className={styles.modalBody}> |
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.
Why are we using ModalBody
instead of the Modal
component?
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.
We use Modal
, it's defined in ModalService
. And later wraps the modal's content with ModalBody
airbyte/airbyte-webapp/src/hooks/services/Modal/ModalService.tsx
Lines 41 to 57 in 7c4980b
return ( | |
<modalServiceContext.Provider value={service}> | |
{children} | |
{modalOptions && ( | |
<Modal | |
title={modalOptions.title} | |
size={modalOptions.size} | |
onClose={modalOptions.preventCancel ? undefined : () => resultSubjectRef.current?.next({ type: "canceled" })} | |
> | |
<modalOptions.content | |
onCancel={() => resultSubjectRef.current?.next({ type: "canceled" })} | |
onClose={(reason) => resultSubjectRef.current?.next({ type: "closed", reason })} | |
/> | |
</Modal> | |
)} | |
</modalServiceContext.Provider> | |
); |
…onPage/components/DestinationForm.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>
# Conflicts: # airbyte-webapp/src/views/Connector/ServiceForm/components/Controls/ConnectorServiceTypeControl/ConnectorServiceTypeControl.tsx # airbyte-webapp/src/views/Connector/ServiceForm/components/StartWithDestination/__snapshots__/StartWithDestination.test.tsx.snap # airbyte-webapp/src/views/Connector/ServiceForm/components/WarningMessage.tsx
@edmundito @lmossman I will answer here since some of your comments are similar: 1. Remove
|
I think that approach is fine, though looking at some of the files in your screenshot it doesn't seem like many of them would need changes just because you remove
I see, I think this makes sense. Especially the storybook comment, I definitely see why having a dumb component with just the formatting logic makes it much easier to have in storybook. So I think I'm happy with this approach I also had one more comment here: #17597 (comment) |
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.
Code LGTM, tested locally and the connector form seems to be working as expected 👍
* remove styled components from FormRoot * refactored WarningMessage component * refactored TestingConnectionSpinner component * fixes after merge * replace styled-components with css modules in ConnectorServiceTypeControl components * extend ModalBody to support style overwrite * refactor RequestConnectorModal using new headlessUi Modal component * - refactor FrequentlyUsedDestinations component - update tests - updates storybook * - refactor StartWithDestination component - update tests - updates storybook * renamed FrequentlyUsedDestinationsCard component * move ConnectorForm to RequestConnectorModal * remove spinner from FrequentlyUsedDestinations component * - refactor ConnectorServiceTypeControl component - move utility functions outside of component - move analytics tracking functions to separate component hook * - refactor ServiceForm component - update tests - remove test with checking serviceType * memoize selectProps in ConnectorServiceTypeControl * simplify getting the namespace type * show FormRoot conditionally - if selectedConnectorDefinitionSpecification is provided * - refactor ConnectorCard component * refactor DestinationForm component * remove styled components from FormRoot * refactored WarningMessage component * refactored TestingConnectionSpinner component * fixes after merge * replace styled-components with css modules in ConnectorServiceTypeControl components * extend ModalBody to support style overwrite * refactor RequestConnectorModal using new headlessUi Modal component * - refactor FrequentlyUsedDestinations component - update tests - updates storybook * - refactor StartWithDestination component - update tests - updates storybook * renamed FrequentlyUsedDestinationsCard component * move ConnectorForm to RequestConnectorModal * remove spinner from FrequentlyUsedDestinations component * - refactor ConnectorServiceTypeControl component - move utility functions outside of component - move analytics tracking functions to separate component hook * - refactor ServiceForm component - update tests - remove test with checking serviceType * memoize selectProps in ConnectorServiceTypeControl * simplify getting the namespace type * show FormRoot conditionally - if selectedConnectorDefinitionSpecification is provided * - refactor ConnectorCard component * refactor DestinationForm component * fixes after rebase * fixed optional onDestinationSelect callbacks * add analyticsTrackFunctions to FrequentlyUsedDestinations component * update tests * fix fullWidth Card issue * fix async/await issue * refactor analytics - Source/Destination connector selected * fix onboarding page - octavia's line * wrap analytics track functions with useCallback * move analytics track functions to separate useAnalyticsTrackFunctions file * rename filename to match the contained component name * extend SectionContainer - do not add margin-bottom if container doesn't have any childs * disable ConnectorServiceTypeControl is form is submitting * fix onbording create source/destination steps * hide form via css if selectedConnector is not present * remove comments * fix e2e tests * fix test * bring back the old design (don't split form on separate cards) * Update airbyte-webapp/src/pages/DestinationPage/pages/CreateDestinationPage/components/DestinationForm.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> * rename "additionalDropdownComponent" to "additionalSelectorComponent" * rename "formType" arg to "connectorType" in analytics track functions * fix components import * updated test snapshot * update RequestConnectorModal: move Formik inside the ModalBody * fix PR comments: rename utilityFunctions.tsx to utils.tsx * move StartWithDestination component to DestinationForm * remove hiding empty form functionality since we don't split it on multiple cards * update comment regarding the serviceType prop Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>
What
Closes #13056
Closes #17666
Design
How
Move
<ConnectorServiceTypeControl />
outside of the Formik and its Context involves changes in almost all related Source/Destination creation form components.Reading order(moving from bottom up)
Major
<RequestConnectorModal />
- was part of<ServiceForm />
, but actually it relates to<ConnectorServiceTypeContol />
directly. What was done:<RequestConnectorModal />
and<ConnectorForm/>
into one component<ConnectorServiceTypeContol />
. What was done:useAnalyticsTrackFunctions.tsx
file alongside the<ConnectorServiceTypeContol />
component, since they are used in one place.<RequestConnectorModal />
and use the new Modal service to control the<RequestConnectorModal />
modal instead of toggling the state in<ServiceForm />
<ServiceForm />
. What was done:<ConnectorServiceTypeContol />
was moved out on one level higher(outside Formik form)<RequestConnectorModal />
,<FrequentlyUsedDestinations />
and<StartWithDestination />
on the right layer(<DestinationForm/>
)<ServiceForm />
to support optional render components in the form (<FrequentlyUsedDestinations />
and<StartWithDestination />
will be passed this way)<FormRoot />
. What was done:<ConnectorCard />
- main Creation/Edit Connector form component, it combines all together. What was done:<ConnectorServiceTypeContol />
here<ConnectorCard />
to support optional render components in the form (<FrequentlyUsedDestinations />
and<StartWithDestination />
will be passed this way as props)<DestinationForm />
- and here we make our<ConnectorCard />
specific via props. What was done:<FrequentlyUsedDestinations />
and<StartWithDestination />
as a props<ConnectorServiceTypeContol />
componentMinor
<StartWithDestination />
- was added additional 'template' layer<StartWithDestinationCard />
. The<StartWithDestinationCard />
it's just a render template and responsible for rendering the passed props. What was done:<StartWithDestination />
and don't mix it with the render template<FrequentlyUsedDestinations />
- Same here: new 'template' layer `. What was done:<FrequentlyUsedDestinations />
and don't mix it with the render template<DestinationForm />
,<DestinationStep />
,<SourceForm />
,<SourceStep />
- the all had analytics tracking functions which actually was a code duplication. And since they all were tracking the same activity - "Connector selected", this tracking function was moved to a related place -<ConnectorServiceTypeContol />
<TestingConnectionSpinner />
. What was done:<WarningMessage />
. What was done:<ModalBody />
. What was done: