🪟 🔧 Refactor FrequentlyUsedDestinations component #19019

letiescanciano · 2022-11-07T10:30:09Z

What

We need to reuse this component in an experiment to suggest sources.
This PR covers the refactor of FrequentlyUsedDestinations component so it's not destination-dependant and can be
used later on.

Demo: https://www.loom.com/share/c207ab2a53c146bd8e4fe57a57660a6b

How

Refactor component to be reused
Rename files

Recommended reading order

I separated the changes in 2 commits to facilitate the review.

Starts with the first commit which only include changes inside files, mainly components name and logic extraction into hook.
File rename

letiescanciano · 2022-11-07T10:33:25Z

...s/Connector/ServiceForm/components/FrequentlyUsedDestinations/FrequentlyUsedDestinations.tsx


-interface FrequentlyUsedDestinationsProps {
+export interface FrequentlyUsedConnectorsProps {


This export isn't needed, will be removed in the last commit

We need to reuse this component in an experiment to suggest sources. This PR covers the refactor so it's not destination-dependant and can be used later on. Demo: https://www.loom.com/share/c207ab2a53c146bd8e4fe57a57660a6b

joeykmh

Code looks good to me! Tested locally and it works as expected. I did notice the storybook doesn't seem to work - would be nice to fix that 🙂

...bapp/src/views/Connector/ConnectorForm/components/FrequentlyUsedConnectors/index.stories.tsx

airbyte-webapp/src/test-utils/mock-data/mockFrequentlyUsedDestinations.ts

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

...Connector/ConnectorForm/components/FrequentlyUsedConnectors/FrequentlyUsedConnectorsCard.tsx

…nent * master: 🪟 🎉 Enable frontend validation for <1hr syncs (cloud) #19028 Stream returns AirbyteMessages (#18572) 🎉 New Source - Recruitee [low-code SDK] (#18671) 🎉 New source: Breezometer [low-code cdk] (#18650) Check disabled connections after protocol update (#18990) Simple default replication worker refactor (#19002) 🎉 New Source: Visma e-conomic (#18595) 🎉 New Source: Fastbill (#18593) Bmoric/extract state api (#18980) 🎉 New Source: Zapier Supported Storage (#18442) 🎉 New source: Klarna (#18385) `AirbyteEstimateTraceMessage` (#18875) Extract source definition api (#18977) [low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file (#18411) 🐛 Source HubSpot: fix property scopes (#18624) Ensure that only 6-character hex values are passed to monaco editor (#18943)

dizel852

Did a review, overall - happy to see one more reusable component. 🥳
But still need to polish some parts, left a comments.

dizel852 · 2022-11-08T10:33:43Z

.../views/Connector/ConnectorForm/components/FrequentlyUsedConnectors/useSuggestedConnectors.ts

+
+import { ConnectorCard } from "../../types";
+
+interface Props {


Suggested change

interface Props {

interface useSuggestedConnectorsProps {

We prefer to use the full name of the interface: https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/STYLEGUIDE.md#component-props

.../views/Connector/ConnectorForm/components/FrequentlyUsedConnectors/useSuggestedConnectors.ts

dizel852 · 2022-11-08T11:06:04Z

airbyte-webapp/src/views/Connector/ConnectorForm/types.ts

+  "sourceDefinitionId" | "name" | "icon" | "releaseStage"
+>;
+
+export type ConnectorCard = (DestinationConnectorCard | SourceConnectorCard) & { id: string };


I'm not sure ConnectorCard it's the best name for this kind of interface, since we already have the ConnectorCard component.
I guess would be better to rely on the return type of the useSuggestedConnectors hook, and name it as is - SuggestedConnector. Also, we can omit destinationDefinitionId and sourceDefinitionId since we use just id(connectorDefinitionId in a comment above)

Suggested change

export type ConnectorCard = (DestinationConnectorCard | SourceConnectorCard) & { id: string };

export type SuggestedConnector = (

| Omit<DestinationConnectorCard, "destinationDefinitionId">

| Omit<SourceConnectorCard, "sourceDefinitionId">

) & { connectorDefinitionId: string };

Love it! Thanks for the suggestion!!

letiescanciano · 2022-11-08T11:15:36Z

@dizel852 sorry for merging so fast before your review. I will include your changes on my next PR which will be using this component.
Thanks a lot for the review!

dizel852 · 2022-11-08T11:18:15Z

@letiescanciano Thanks! 🙏

timroes · 2022-11-08T11:24:14Z

@letiescanciano since the next PR using it will be an experimentation PR, could you please make the changes in a separate one, so that we have the experimentation PR scoped to only the experiment

* 🪟 🔧 Refactor FrequentlyUsedDestinations We need to reuse this component in an experiment to suggest sources. This PR covers the refactor so it's not destination-dependant and can be used later on. Demo: https://www.loom.com/share/c207ab2a53c146bd8e4fe57a57660a6b * PR comments

letiescanciano requested a review from a team as a code owner November 7, 2022 10:30

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp team/growth labels Nov 7, 2022

letiescanciano requested a review from timroes November 7, 2022 10:30

letiescanciano commented Nov 7, 2022

View reviewed changes

letiescanciano changed the title ~~Leti/refactor suggested destinations component~~ 🪟 🔧 Refactor FrequentlyUsedDestinations component Nov 7, 2022

letiescanciano added 2 commits November 7, 2022 19:04

🪟 🔧 Refactor FrequentlyUsedDestinations

a3b2c48

We need to reuse this component in an experiment to suggest sources. This PR covers the refactor so it's not destination-dependant and can be used later on. Demo: https://www.loom.com/share/c207ab2a53c146bd8e4fe57a57660a6b

update filenames

5040772

letiescanciano force-pushed the leti/refactor-suggested-destinations-component branch from d9ae8dd to 5040772 Compare November 7, 2022 18:15

joeykmh approved these changes Nov 8, 2022

View reviewed changes

letiescanciano added 2 commits November 8, 2022 10:58

PR comments

aa1442e

letiescanciano merged commit c1a8169 into master Nov 8, 2022

letiescanciano deleted the leti/refactor-suggested-destinations-component branch November 8, 2022 10:47

dizel852 requested changes Nov 8, 2022

View reviewed changes

letiescanciano mentioned this pull request Nov 8, 2022

🪟 🔧 FrequentlyUsedConnectors improvements #19125

Merged

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

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

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🪟 🔧 Refactor FrequentlyUsedDestinations component #19019

🪟 🔧 Refactor FrequentlyUsedDestinations component #19019

Uh oh!

letiescanciano commented Nov 7, 2022

Uh oh!

letiescanciano Nov 7, 2022 •

edited

Loading

Uh oh!

joeykmh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dizel852 left a comment

Uh oh!

dizel852 Nov 8, 2022

Uh oh!

dizel852 Nov 8, 2022

Uh oh!

Uh oh!

dizel852 Nov 8, 2022

Uh oh!

letiescanciano Nov 8, 2022

Uh oh!

letiescanciano commented Nov 8, 2022

Uh oh!

dizel852 commented Nov 8, 2022

Uh oh!

timroes commented Nov 8, 2022

Uh oh!

Uh oh!


		interface FrequentlyUsedDestinationsProps {
		export interface FrequentlyUsedConnectorsProps {


		import { ConnectorCard } from "../../types";

		interface Props {

-export type ConnectorCard = (DestinationConnectorCard | SourceConnectorCard) & { id: string };
+export type SuggestedConnector = (
+  | Omit<DestinationConnectorCard, "destinationDefinitionId">
+  | Omit<SourceConnectorCard, "sourceDefinitionId">
+) & { connectorDefinitionId: string };

🪟 🔧 Refactor FrequentlyUsedDestinations component #19019

🪟 🔧 Refactor FrequentlyUsedDestinations component #19019

Uh oh!

Conversation

letiescanciano commented Nov 7, 2022

What

How

Recommended reading order

Uh oh!

letiescanciano Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joeykmh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dizel852 left a comment

Choose a reason for hiding this comment

Uh oh!

dizel852 Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

dizel852 Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dizel852 Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

letiescanciano Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

letiescanciano commented Nov 8, 2022

Uh oh!

dizel852 commented Nov 8, 2022

Uh oh!

timroes commented Nov 8, 2022

Uh oh!

Uh oh!

letiescanciano Nov 7, 2022 •

edited

Loading