-
Notifications
You must be signed in to change notification settings - Fork 248
MNTOR-4592 + MNTOR-4530 - Add feature flag to UpsellBadge #5981
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
base: main
Are you sure you want to change the base?
Conversation
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.
Question: Would the UpsellDialog
not need to show only when dialogState.isOpen && !props.enabledFeatureFlags.includes("DisableOneRepScans")
?
@@ -309,6 +309,7 @@ export const DashboardTopBannerContent = (props: DashboardTopBannerProps) => { | |||
monthlySubscriptionUrl={monthlySubscriptionUrl} | |||
yearlySubscriptionUrl={yearlySubscriptionUrl} | |||
subscriptionBillingAmount={subscriptionBillingAmount} | |||
enabledFeatureFlags={props.enabledFeatureFlags} |
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.
It seems like we also need to add the prop enabledFeatureFlags
to the story in UpsellCta.stories.tsx
as it seems to break at the moment due to this change.
@@ -53,6 +53,7 @@ const UpsellCtaWrapper = (props: UpsellCtaWrapperProps) => { | |||
monthlySubscriptionUrl={monthlySubscriptionUrl} | |||
yearlySubscriptionUrl={yearlySubscriptionUrl} | |||
subscriptionBillingAmount={subscriptionBillingAmount} | |||
enabledFeatureFlags={props.enabledFeatureFlags} |
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.
@flozia Didn't I already add it here?
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.
@flozia The upsell badge can still be visible, but if the flag is enabled it should show the waitlist popup instead. |
@@ -53,6 +53,7 @@ const UpsellCtaWrapper = (props: UpsellCtaWrapperProps) => { | |||
monthlySubscriptionUrl={monthlySubscriptionUrl} | |||
yearlySubscriptionUrl={yearlySubscriptionUrl} | |||
subscriptionBillingAmount={subscriptionBillingAmount} | |||
enabledFeatureFlags={props.enabledFeatureFlags} |
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.
{dialogState.isOpen && | ||
(props.enabledFeatureFlags.includes("DisableOneRepScans") ? ( |
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.
@flozia The upsell badge can still be visible, but if the flag is enabled it should show the waitlist popup instead.
@codemist The newly introduced condition now says: “If the dialog is open AND the feature flag DisableOneRepScans
is enabled, show the WaitlistDialog
. Otherwise, show the UpsellDialog
”. Doesn’t that mean that if dialogState.isOpen === false
we show the UpsellDialog
, which seems like it is not the intended behaviour?
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.
The dialog (Upsell or Waitlist) is only shown when dialogState.isOpen
is true
. If the dialog is closed, nothing is rendered. The &&
in JSX ensures that neither dialog appears unless the dialog is open, so there’s no risk of the UpsellDialog
showing when it shouldn’t.
I don't think the failing test here has anything to do with the changes. |
References:
Jira: MNTOR-4592 and MNTOR-4530
Figma:
Description
Screenshot (if applicable)
Not applicable.
How to test
Checklist (Definition of Done)