Fix: Place schedule demo button on the discount banner when there is one #57092
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
dubielzyk-expensify and
fedirjh
and removed request for
a team
|
@dubielzyk-expensify @fedirjh One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
truph01
changed the title
|
I think we want the gap between buttons to be 8px - it looks a bit larger right now: |
|
Not a blocker at all but: is it possible to get the admin button in the room header before the room animates in? It feels slightly jarring: CleanShot.2025-02-19.at.08.04.58.mp4 |
|
@truph01 The dismiss button is not displayed for me 
|
fedirjh
suggested changes
Feb 19, 2025
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
I believe the dismiss icon is only showed per a certain condition, like if the trial is past X days or something. I think @dannymcclain knows what the deal is there! |
Yeah I do believe dismiss is only shown under certain conditions, but TBH I don't know exactly what those conditions are. @ryanschaffer can you clarify here? |
PR is updated to address it. Result:
PR is updated to address it. Result:
|
|
Nice, this is feeling pretty good to me. |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
|
@Expensify/design Is this expected ? Shouldn’t the button be displayed in full size in mobile ? 
|
|
Hmm no, in that case it should go full width. |
|
@truph01 This code change should fix it: Code Diffdiff --git a/src/pages/home/HeaderView.tsx b/src/pages/home/HeaderView.tsx
index 5cae1d65ccc..0c1f013d004 100644
--- a/src/pages/home/HeaderView.tsx
+++ b/src/pages/home/HeaderView.tsx
@@ -214,18 +214,11 @@ function HeaderView({report, parentReportAction, onNavigationMenuButtonClicked,
onPress={() => {
openExternalLink(account?.guideDetails?.calendarLink ?? '');
}}
- style={shouldUseNarrowLayout && shouldShowGuideBookingButtonInEarlyDiscountBanner && [styles.flex1]}
+ style={[styles.flex1]}
icon={CalendarSolid}
/>
);
- const getGuideBookButtonStyles = () => {
- if (isChatUsedForOnboarding) {
- return [styles.pb3, styles.pl5, styles.w50, styles.pr1];
- }
- return [styles.pb3, styles.ph5];
- };
-
return (
<>
<View
@@ -338,7 +331,7 @@ function HeaderView({report, parentReportAction, onNavigationMenuButtonClicked,
</View>
)}
</PressableWithoutFeedback>
- <View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter]}>
+ <View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter, styles.gap2]}>
{!shouldShowGuideBookingButtonInEarlyDiscountBanner && shouldShowGuideBooking && !shouldUseNarrowLayout && guideBookingButton}
{!shouldUseNarrowLayout && !shouldShowDiscount && isChatUsedForOnboarding && (
<FreeTrial
@@ -369,10 +362,8 @@ function HeaderView({report, parentReportAction, onNavigationMenuButtonClicked,
)}
</View>
{!isParentReportLoading && !isLoading && canJoin && shouldUseNarrowLayout && <View style={[styles.ph5, styles.pb2]}>{joinButton}</View>}
- <View style={isChatUsedForOnboarding && !shouldShowDiscount && shouldShowGuideBooking && [styles.dFlex, styles.flexRow]}>
- {!shouldShowGuideBookingButtonInEarlyDiscountBanner && !isLoading && shouldShowGuideBooking && shouldUseNarrowLayout && (
- <View style={getGuideBookButtonStyles()}>{guideBookingButton}</View>
- )}
+ <View style={isChatUsedForOnboarding && !shouldShowDiscount && shouldShowGuideBooking && [styles.dFlex, styles.flexRow, styles.gap2, styles.pb3, styles.ph5]}>
+ {!shouldShowGuideBookingButtonInEarlyDiscountBanner && !isLoading && shouldShowGuideBooking && shouldUseNarrowLayout && guideBookingButton}
{!isLoading && !shouldShowDiscount && isChatUsedForOnboarding && shouldUseNarrowLayout && (
<FreeTrial
pressable
diff --git a/src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx b/src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
index 705b83b7796..cefa4ba0604 100644
--- a/src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
+++ b/src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
@@ -54,24 +54,25 @@ function EarlyDiscountBanner({isSubscriptionPage, GuideBookingButton, onDismisse
}, [firstDayFreeTrial]);
const dismissButton = useMemo(
- () => (
- <Tooltip text={translate('common.close')}>
- <PressableWithFeedback
- onPress={() => {
- setIsDismissed(true);
- onDismissedDiscountBanner?.();
- }}
- role={CONST.ROLE.BUTTON}
- accessibilityLabel={translate('common.close')}
- >
- <Icon
- src={Expensicons.Close}
- fill={theme.icon}
- />
- </PressableWithFeedback>
- </Tooltip>
- ),
- [theme.icon, translate, onDismissedDiscountBanner],
+ () =>
+ discountInfo?.discountType === 50 && (
+ <Tooltip text={translate('common.close')}>
+ <PressableWithFeedback
+ onPress={() => {
+ setIsDismissed(true);
+ onDismissedDiscountBanner?.();
+ }}
+ role={CONST.ROLE.BUTTON}
+ accessibilityLabel={translate('common.close')}
+ >
+ <Icon
+ src={Expensicons.Close}
+ fill={theme.icon}
+ />
+ </PressableWithFeedback>
+ </Tooltip>
+ ),
+ [discountInfo?.discountType, translate, theme.icon, onDismissedDiscountBanner],
);
const rightComponent = useMemo(() => {
@@ -85,7 +86,7 @@ function EarlyDiscountBanner({isSubscriptionPage, GuideBookingButton, onDismisse
text={translate('subscription.billingBanner.earlyDiscount.claimOffer')}
onPress={() => Navigation.navigate(ROUTES.SETTINGS_SUBSCRIPTION.getRoute(Navigation.getActiveRoute()))}
/>
- {discountInfo?.discountType === 25 && (shouldUseNarrowLayout ? undefined : dismissButton)}
+ {!shouldUseNarrowLayout && dismissButton}
</View>
);
}, [
@@ -99,7 +100,6 @@ function EarlyDiscountBanner({isSubscriptionPage, GuideBookingButton, onDismisse
styles.gap2,
styles.flex1,
translate,
- discountInfo?.discountType,
GuideBookingButton,
dismissButton,
]);
@@ -125,7 +125,7 @@ function EarlyDiscountBanner({isSubscriptionPage, GuideBookingButton, onDismisse
<Text>{translate('subscription.billingBanner.earlyDiscount.onboardingChatTitle.phrase2', {discountType: discountInfo?.discountType})}</Text>
</Text>
)}
- {discountInfo?.discountType === 25 && (shouldUseNarrowLayout ? dismissButton : undefined)}
+ {shouldUseNarrowLayout && dismissButton}
</View>
);
diff --git a/src/pages/settings/Subscription/FreeTrial.tsx b/src/pages/settings/Subscription/FreeTrial.tsx
index 1d82614d66f..3d99372742c 100644
--- a/src/pages/settings/Subscription/FreeTrial.tsx
+++ b/src/pages/settings/Subscription/FreeTrial.tsx
@@ -56,7 +56,7 @@ function FreeTrial({badgeStyles, pressable = false, addSpacing = false, success
/>
);
- return addSpacing ? <View style={inARow ? [styles.pb3, styles.pr5, styles.w50, styles.pl1] : [styles.pb3, styles.ph5]}>{freeTrial}</View> : freeTrial;
+ return addSpacing ? <View style={inARow ? [styles.flex1] : [styles.pb3, styles.ph5]}>{freeTrial}</View> : freeTrial;
}
FreeTrial.displayName = 'FreeTrial';
|
|
Working on the fix. |
|
@fedirjh Thanks for your suggested code in this comment. However, I previously investigated the issue and found a solution that only requires a one-line change, which helps minimize the risk of regression. Let's first test whether my change in this commit resolves the issue. If it doesn't, I'll proceed with your suggested changes. Below are the results after applying my fix:
|
fedirjh
reviewed
Feb 25, 2025
There was a problem hiding this comment.
@truph01 Thank you for the update. I’ve noticed that there is still no gap between the buttons in the wide-screen views. The code I provided here should fix and simplify thi. To further explain my changes, I observed that we are not utilizing styles correctly: instead of using flex and gap prop styles, we were relying on padding and margin to space items, which doesn’t look ideal to me.
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Outdated
Show resolved
Hide resolved
|
@fedirjh I applied your code changes here, which removes the getGuideBookButtonStyles function. However, I encountered an issue when applying that change:
Result: |
MonilBhavsar
approved these changes
Feb 26, 2025
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
40 tasks
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.1.7-1 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.1.7-2 🚀
|
8 tasks
50 tasks
| text={translate('getAssistancePage.scheduleADemo')} | ||
| onPress={() => { | ||
| openExternalLink(account?.guideDetails?.calendarLink ?? ''); | ||
| }} | ||
| style={!shouldUseNarrowLayout && isChatUsedForOnboarding && styles.mr2} | ||
| style={shouldUseNarrowLayout && shouldShowGuideBookingButtonInEarlyDiscountBanner && [styles.flex1]} |
There was a problem hiding this comment.
This PR introduced a style bug, detailed here #58974
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #55879
PROPOSAL: #55879 (comment)
Tests
#adminroom.Offline tests
QA Steps
#adminroom.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-02-19.at.17.36.40.mov
Android: mWeb Chrome
Screen.Recording.2025-02-19.at.17.34.56.mov
iOS: Native
Screen.Recording.2025-02-19.at.17.26.58.mov
iOS: mWeb Safari
Screen.Recording.2025-02-19.at.17.09.10.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-19.at.16.56.32.mov
MacOS: Desktop
Screen.Recording.2025-02-19.at.17.32.47.mov