🪟 🎨 Display free tag on release stage pills for users enrolled in the free connectors program #21892

ambirdsall · 2023-01-25T22:25:28Z

What

Displays the free badge on release stage pills if:

the user is enrolled in the free connector program
the release stage is "alpha" or "beta"

Closes #21812.

How, and some additional considerations

Defines a new FeatureItem.FreeConnectorProgram flag and adds it to defaultCloudFeatures. As of now, there is no corresponding featureService.FREE_CONNECTOR_PROGRAM flag in LaunchDarkly, since all our dynamic override needs are met by the temporary experiment flag we're using to roll the feature out.
Extracts a new, self-contained FreeTag component to encapsulate all interaction with useFreeConnectorProgram, and by extension the cloud-only endpoint that backs it, behind a component boundary. This means OSS builds always get to short-circuit that API via conditionals like { useFeature(FeatureItem.FreeConnectorProgram) && <FreeTag releaseStage={releaseStage} /> }; closes Refactor useFreeConnectorProgram() so it does not fire in OSS #21872
- Another benefit is to the test code: the initial definition relied a showFreeTag prop, largely as a way to avoid a big, pointless rewrite of some unit test setup code. This pattern dramatically simplified the extra setup needed, making it once again reasonable to centralize the "should the free tag render for this workspace/connector?" logic within the pills.
- It's defined as a totally self-contained component within the FreeConnectorProgram directory, rather than as a helper component within ReleaseStageBadge.tsx in order to deal with the fact that there are two separate, almost-identical implementations of release stage pills. Obvious possible follow-up refactoring there.

But how does it look

From the connection page title, embedded in a ReleaseStageBadge:

From the new source/destination dropdown, embedded in a StageLabel (is there any reason not to just use a ReleaseStageBadge here?):

ambirdsall · 2023-01-25T22:45:39Z

airbyte-webapp/src/components/ReleaseStageBadge/ReleaseStageBadge.tsx

@@ -37,13 +33,10 @@ export const ReleaseStageBadge: React.FC<ReleaseStageBadgeProps> = ({
      <div
        className={classNames(styles.pill, {
          [styles["pill--small"]]: small,
-          [styles["pill--contains-tag"]]: showFreeTag,


One downside of putting all code shared with OSS on the other side of a component boundary: we no longer have a good way to conditionally change the padding-right from 6px to 2px based on whether a free tag is displayed. Instead, the FreeTag component uses a negative margin-right; that's a hacky solution, though, do let me know if you have a better idea.

Tbh I don't think the negative margin-right is that bad, but another idea:

{!fcpEnabled && <div className={classNames(styles.pill, { [styles["pill--small"]]: small })} > <FormattedMessage id={`connector.releaseStage.${stage}`} /> </div>} {fcpEnabled && <FCPReleaseStageBadge stage={stage} />}

Where <FCPReleaseStageBadge> renders the whole pill + conditionally a free tag. It's duplicate code, but I think it's warranted to have a clean split between "feature enabled" and "feature disabled".

I do like that a bit better, but I'm inclined to save that for later, when consolidating the three implementations of stage pills (there's another copy-paste StageLabel component in cloud/components/experiments/ , used by the sign up page; I left that untouched for now since not-yet-created accounts will never be enrolled). If the overall design of the status pills changes, as things stand, it would be way too easy to overlook one of those.

josephkmh

Looks good! Left a few ideas for improvements, but nothing blocking 👍

josephkmh · 2023-01-26T10:16:55Z

airbyte-webapp/src/components/EntityTable/components/ConnectorCell.tsx

@@ -12,7 +12,7 @@ interface IProps {
 const Content = styled.div<{ enabled?: boolean }>`
  display: flex;
  align-items: center;
-  color: ${({ theme, enabled }) => (!enabled ? theme.greyColor40 : "inheret")};


josephkmh · 2023-01-26T10:25:06Z

airbyte-webapp/src/components/ReleaseStageBadge/ReleaseStageBadge.tsx

@@ -37,13 +33,10 @@ export const ReleaseStageBadge: React.FC<ReleaseStageBadgeProps> = ({
      <div
        className={classNames(styles.pill, {
          [styles["pill--small"]]: small,
-          [styles["pill--contains-tag"]]: showFreeTag,


Tbh I don't think the negative margin-right is that bad, but another idea:

{!fcpEnabled && <div className={classNames(styles.pill, { [styles["pill--small"]]: small })} > <FormattedMessage id={`connector.releaseStage.${stage}`} /> </div>} {fcpEnabled && <FCPReleaseStageBadge stage={stage} />}

Where <FCPReleaseStageBadge> renders the whole pill + conditionally a free tag. It's duplicate code, but I think it's warranted to have a clean split between "feature enabled" and "feature disabled".

josephkmh · 2023-01-26T11:35:58Z

airbyte-webapp/src/packages/cloud/components/experiments/FreeConnectorProgram/FreeTag.tsx

+  const { enrollmentStatusQuery } = useFreeConnectorProgram();
+  const { isEnrolled } = enrollmentStatusQuery.data || {};
+  const { formatMessage } = useIntl();
+  const freeStages: Array<ReleaseStage | undefined> = ["alpha", "beta"];


Suggested change

const freeStages: Array<ReleaseStage | undefined> = ["alpha", "beta"];

const freeStages: ReleaseStage[] = ["alpha", "beta"];

I can't imagine why we would want undefined in this array?

The type checker is very uncomfortable relying on the fact that (someArray as Array<T>).includes(undefined) === false for all types T, even though it's totally idiomatic js; so the elements of freeStages have to match the type of isEnrolled. Absent type errors, I would have just used an inline array literal (["alpha", "beta"].includes(releaseStage)).

The other reasonable alternative would be to cast isEnrolled, as freeStages.includes(releaseStage as ReleaseStage); I think it's an equivalently good solution for this case, but a general preference for avoiding casts was the tie-breaker (because every time I read one I have to consider the possibility of human error in addition to all the types).

Not sure I quite follow - the change I suggested passes my TS checker just fine (so does ["alpha", "beta"].includes(releaseStage), for that matter). What error are you seeing there?

...none, now 😳. I certainly remember only adding that | undefined in response to some Argument of type ReleaseStage | undefined is not assignable to parameter of type ReleaseStage error, but now it even type-checks if I go back to the inline array of strings I tried initially. I feel like I'm taking crazy pills, but I'll take the cleaner code, I guess.

...ctor/ConnectorForm/components/FrequentlyUsedConnectors/FrequentlyUsedConnectorsCard.test.tsx

This is distinct from the LaunchDarkly-backed experiment we're referencing within `cloud/components/experiments/FreeConnectorProgram`: it's primary use is to prevent triggering cloud-only API calls in the OSS build.

It does seem as though we should consolidate on a single release stage pill implementation! That will have to wait for a follow-up, though.

"...and your little type assertion, too!"

octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 25, 2023

ambirdsall commented Jan 25, 2023

View reviewed changes

ambirdsall force-pushed the alex/display-free-pills branch from 2f87a61 to 06e1f85 Compare January 25, 2023 23:28

ambirdsall requested review from josephkmh and teallarson January 25, 2023 23:30

ambirdsall marked this pull request as ready for review January 26, 2023 00:14

josephkmh approved these changes Jan 26, 2023

View reviewed changes

ambirdsall added 6 commits January 26, 2023 09:27

Add FeatureItem.FreeConnectorProgram

406aeb5

This is distinct from the LaunchDarkly-backed experiment we're referencing within `cloud/components/experiments/FreeConnectorProgram`: it's primary use is to prevent triggering cloud-only API calls in the OSS build.

Extract shareable FreeTag for various release stage pills

17eca44

It does seem as though we should consolidate on a single release stage pill implementation! That will have to wait for a follow-up, though.

Fix typo (s/inheret/inherit/)

181c4f8

Prefer useFeature to useFreeConnectorProgram in OSS components

a1d5fa8

Use TestWrapper for more future-proof test renders

87387c8

Remove unneed intermediate variable

f6dd490

"...and your little type assertion, too!"

ambirdsall force-pushed the alex/display-free-pills branch from 89a6f2c to f6dd490 Compare January 26, 2023 17:30

ambirdsall merged commit 4068bbe into master Jan 26, 2023

ambirdsall deleted the alex/display-free-pills branch January 26, 2023 18:20

This was referenced Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #21954

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #21991

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #22045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎨 Display free tag on release stage pills for users enrolled in the free connectors program #21892

🪟 🎨 Display free tag on release stage pills for users enrolled in the free connectors program #21892

ambirdsall commented Jan 25, 2023 •

edited

Loading

ambirdsall Jan 25, 2023

josephkmh Jan 26, 2023

ambirdsall Jan 26, 2023

josephkmh left a comment

josephkmh Jan 26, 2023

josephkmh Jan 26, 2023

josephkmh Jan 26, 2023

ambirdsall Jan 26, 2023

josephkmh Jan 26, 2023 •

edited

Loading

ambirdsall Jan 26, 2023

	const freeStages: Array<ReleaseStage \| undefined> = ["alpha", "beta"];
	const freeStages: ReleaseStage[] = ["alpha", "beta"];

🪟 🎨 Display free tag on release stage pills for users enrolled in the free connectors program #21892

🪟 🎨 Display free tag on release stage pills for users enrolled in the free connectors program #21892

Conversation

ambirdsall commented Jan 25, 2023 • edited Loading

What

How, and some additional considerations

But how does it look

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh 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

josephkmh Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambirdsall commented Jan 25, 2023 •

edited

Loading

josephkmh Jan 26, 2023 •

edited

Loading