implement billing banner #43267

pasyukevich · 2024-06-07T12:05:19Z

Details

To access this newly created component, paste the following link into the browser

https://dev.new.expensify.com:8082/settings/subscription

or add this effect to InitialSettingsPage.tsx

useEffect(() => {
    Navigation.navigate(ROUTES.SETTINGS_SUBSCRIPTION)
}, [])

Add banner with text and other parameters to the section in the CardSection.tsx

import BillingBanner from './BillingBanner';

 <Section
            title={translate('subscription.cardSection.title')}
            subtitle={translate('subscription.cardSection.subtitle')}
            isCentralPane
            titleStyles={styles.textStrong}
            banner={
                <BillingBanner
                    title="Your card couldn’t be charged!"
                    subtitle="Before retrying, please call your bank directly to authorize Expensify charges and remove any holds. Otherwise, try adding a different payment card."
                    isError // to show error icon
                    shouldShowRedDotIndicator // to show red dot indicator
                />
            }
            subtitleMuted
        >

Fixed Issues

$ #42430
PROPOSAL:

Tests

Login into the app
Open 'Subscription' page
Verify that the section is rendered correctly with the error showed
Verify that the section is rendered correctly with success

Verify that no errors appear in the JS console

Offline tests

QA Steps

No QA

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

pasyukevich · 2024-06-07T13:53:09Z

This PR introduces only the UI for the billing banner

All translations and logic of showing will be implemented in the scope of this issue.

melvin-bot · 2024-06-07T13:55:05Z

@ishpaul777 Please 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]

shawnborton · 2024-06-07T14:23:02Z

Looks like the line height is off:

For the smaller text below, can we make sure it uses a line height of 20px, which should be the default line height for our normal text?

In Figma, we have a 2px gap between the headline and smaller text below it seems too. cc @Expensify/design for the gut check there.

trjExpensify · 2024-06-07T14:27:26Z

@mananjadhav is going to review this one!

dannymcclain · 2024-06-07T14:40:05Z

For the smaller text below, can we make sure it uses a line height of 20px, which should be the default line height for our normal text?

Yeah, headline should use our Page Header type style set in our default text color, and the message should use our default Text type style set in our text-supporting color.

In Figma, we have a 2px gap between the headline and smaller text below it seems too. cc @Expensify/design for the gut check there.

I don't think this is correct. I tried to clean all these up but I think with all the permutations in the file this slipped through the cracks. I think there should be 0 gap between headline and smaller text below. Thoughts on that?

shawnborton · 2024-06-07T14:41:54Z

Cool, that works for me!

dannymcclain · 2024-06-07T14:46:41Z

Cool, that works for me!

Ok cool. I've gotten them all cleaned up in Figma too.

pasyukevich · 2024-06-07T15:21:05Z

@shawnborton @dannymcclain

Updated with usage of styles.headerText

shawnborton · 2024-06-07T15:28:59Z

Can you check the line height of this text here?

I wonder if it just feels big because of that 21px line height bug we need to solve :sigh:

pasyukevich · 2024-06-07T15:45:57Z

@shawnborton updated

shawnborton · 2024-06-07T16:01:53Z

src/styles/index.ts

@@ -2848,6 +2848,11 @@ const styles = (theme: ThemeColors) =>
            width: variables.iconSizeExtraLarge,
        },

+        billingBannerSubtitle: {


Do we really need to write an ultra specific style here? Why not just reuse existing styles here? For example, we have colorMuted that can be added, and I bet we have our lineHeight as a style as well.

Yes, we have colorMuted, but we do not have styles that specify only the lineHeight

Then let's add a generic helper class for lineHeight20... though I think our normal text should automatically have a good line height, right?

Updated

Do you mean a specific style by a normal text?

I'm saying that our standard, "default" text across the app should be at 15px, in our Expensify Neue font, with 20px line height. I'm not sure if all default text gets those styles or not though.

Currently, we have 21px line height as a default
https://github.com/Expensify/App/blob/5c86b722a8df1c2cf11ee9d3a3e62c6e69a05376/src/styles/index.ts#L231C9-L231C22

Got it. Let's just leave this as-is then, and I'll create a follow up issue to fix our line height from being 21px.

Value of the fontSizeNormalHeight -

App/src/styles/variables.ts

Line 66 in 5c86b72

fontSizeNormalHeight: getValueUsingPixelRatio(21, 28),

reverted and removed specific line-height style

shawnborton · 2024-06-10T09:30:55Z

src/styles/index.ts

@@ -455,6 +455,10 @@ const styles = (theme: ThemeColors) =>
            fontWeight: FontUtils.fontWeight.bold,
        },

+        textLineHeightXLarge: {


This feels like a very misleading name for this... I wouldn't consider this line height to be XLarge, I would consider it to be our default line height for our default font size.

Does it mean updating the baseFontStyle or creating a more suitable name?

Can you confirm that our base font style includes line height? And if so, what is the line height? Because maybe we don't actually need to do anything here and we can just use our "default" text styles.

shawnborton · 2024-06-10T12:31:05Z

Sounds good, I think that's the best path forward for now.

pasyukevich · 2024-06-10T16:48:36Z

@mananjadhav Could you review the PR?

mananjadhav · 2024-06-10T16:53:10Z

Getting to this in 10 mins. Just finishing a checklist on another one.

dannymcclain · 2024-06-10T18:27:30Z

I think we do not want that spacing, but let's confirm with @dannymcclain who designed it :)

We do not, thanks!

mananjadhav · 2024-06-10T18:31:49Z

Thanks @dannymcclain @shawnborton .

I think the rest of the comments weren't blockers. Working on the checklist now.

dannymcclain · 2024-06-10T18:52:06Z

Ok I am so sorry to make such a last minute change, but looking at it in the product, I feel like the headline in the banner is just too big.

We currently have it using our Page Header type style which is 17px, but I think it looks a lot better when it uses our Text Strong type style which is 15px. Line-height should stay 20px and weight should stay 700. When it's 17px it just really competes with the card title and the page title, and it's just not great. This is 100% my fault, and again, I apologize for requesting this change so late in the game! 😬

Here's a side by side:

17px (Current)	15px (Suggested)

cc @Expensify/design

Also, we're going to have lots of variations of this little banner with different trial/subscription messaging. Does it make sense to add these style variations now? Or will that come later? The main differences are the background and text colors, but here's an overview of the two basic styles we'll have:

dannymcclain · 2024-06-10T18:54:34Z

Also I think the padding for the banner should just be padding: 16px 20px; We don't need it to have less padding on the bottom than the top.

shawnborton · 2024-06-10T20:44:37Z

No comments from me, I agree with everything you've laid out here and I agree the smaller (normal) font for the headline feels better!

mananjadhav · 2024-06-11T04:21:01Z

@pasyukevich Can you take a look at the design comments?

pasyukevich · 2024-06-11T13:08:29Z

@dannymcclain Sure! I'm already preparing the updates among with trial styles

pasyukevich · 2024-06-11T14:30:20Z

@dannymcclain

updated fonts and offsets

pasyukevich · 2024-06-11T14:31:34Z

Added logic for trial fonts and background

pasyukevich · 2024-06-11T15:37:24Z

@mananjadhav Could you review the updated version?

mananjadhav · 2024-06-11T16:25:04Z

src/pages/settings/Subscription/CardSection/CardSection.tsx

@@ -31,6 +32,15 @@ function CardSection() {
            isCentralPane
            titleStyles={styles.textStrong}
            subtitleMuted
+            banner={
+                <BillingBanner


Can we get rid of this dummy code?

dannymcclain

From a design perspective this looks good. I played around with the banner props to check out the different variations and everything is looking good. Thanks for handling my last minute changes! Definitely still get a proper technical review.

mananjadhav · 2024-06-11T18:07:31Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

desktop-billing-banner.mov

mananjadhav · 2024-06-11T18:34:41Z

I am done with most of the checklist, except for Android. For some reason the Android always loads a blank page for me. Checking.

mananjadhav · 2024-06-11T18:51:35Z

Something's wrong with my Android setup (doesn't work on any branch). I am going to go ahead approve the PR to unblock this.

amyevans

Code and UI LGTM!

Here's Android just to cover our bases:

OSBotify · 2024-06-11T20:38:21Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

amyevans · 2024-06-11T20:59:08Z

Oh FYI I removed the QA steps since Applause will not be able to edit the code to add a fake banner. Once we have real data integrated we can have them look at it

OSBotify · 2024-06-11T22:28:59Z

🚀 Deployed to staging by https://github.com/amyevans in version: 1.4.82-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-06-13T15:31:35Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.82-4 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

pasyukevich force-pushed the feature/billing-banner branch 2 times, most recently from 082aa97 to 1f9390e Compare June 7, 2024 13:42

implement billing banner

bdd0adc

pasyukevich force-pushed the feature/billing-banner branch from 1f9390e to bdd0adc Compare June 7, 2024 13:51

pasyukevich marked this pull request as ready for review June 7, 2024 13:55

pasyukevich requested review from a team as code owners June 7, 2024 13:55

melvin-bot bot removed the request for review from a team June 7, 2024 13:55

melvin-bot bot requested a review from ishpaul777 June 7, 2024 13:55

trjExpensify requested review from mananjadhav and removed request for ishpaul777 June 7, 2024 14:27

fix header

58fe381

fix banner subtitle

2487592

shawnborton reviewed Jun 7, 2024

View reviewed changes

remove specific style

94f0a01

shawnborton reviewed Jun 10, 2024

View reviewed changes

remove specific lineHeight

f0e23c9

shawnborton requested a review from dannymcclain June 10, 2024 18:15

update props, add variables

0dd74d7

update ui

5ffec6f

mananjadhav reviewed Jun 11, 2024

View reviewed changes

dannymcclain previously approved these changes Jun 11, 2024

View reviewed changes

remove dummy code

1baaae0

pasyukevich dismissed dannymcclain’s stale review via 1baaae0 June 11, 2024 17:51

mananjadhav approved these changes Jun 11, 2024

View reviewed changes

melvin-bot bot requested a review from amyevans June 11, 2024 18:51

amyevans approved these changes Jun 11, 2024

View reviewed changes

amyevans merged commit d5f5c73 into Expensify:main Jun 11, 2024
16 of 18 checks passed

github-actions bot mentioned this pull request Jun 11, 2024

Deploy Checklist: New Expensify 2024-06-11 #43515

Closed

59 tasks

melvin-bot bot mentioned this pull request Jun 13, 2024

[HOLD for payment 2024-06-20] [Payment card / Subscription] Implement billing banner (UI) #42430

Closed

implement billing banner #43267

implement billing banner #43267

Conversation

pasyukevich commented Jun 7, 2024 • edited by amyevans Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

pasyukevich commented Jun 7, 2024 • edited Loading

melvin-bot bot commented Jun 7, 2024

shawnborton commented Jun 7, 2024 • edited Loading

trjExpensify commented Jun 7, 2024

dannymcclain commented Jun 7, 2024

shawnborton commented Jun 7, 2024

dannymcclain commented Jun 7, 2024

pasyukevich commented Jun 7, 2024

shawnborton commented Jun 7, 2024

pasyukevich commented Jun 7, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Jun 10, 2024

pasyukevich commented Jun 10, 2024

mananjadhav commented Jun 10, 2024

dannymcclain commented Jun 10, 2024

mananjadhav commented Jun 10, 2024

dannymcclain commented Jun 10, 2024

dannymcclain commented Jun 10, 2024

shawnborton commented Jun 10, 2024

mananjadhav commented Jun 11, 2024

pasyukevich commented Jun 11, 2024

pasyukevich commented Jun 11, 2024

pasyukevich commented Jun 11, 2024

pasyukevich commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannymcclain left a comment

Choose a reason for hiding this comment

mananjadhav commented Jun 11, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

mananjadhav commented Jun 11, 2024

mananjadhav commented Jun 11, 2024

amyevans left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 11, 2024

amyevans commented Jun 11, 2024

OSBotify commented Jun 11, 2024

OSBotify commented Jun 13, 2024

pasyukevich commented Jun 7, 2024 •

edited by amyevans

Loading

pasyukevich commented Jun 7, 2024 •

edited

Loading

shawnborton commented Jun 7, 2024 •

edited

Loading

mananjadhav commented Jun 11, 2024 •

edited

Loading