Skip to content
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.

Sign up for GitHub

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

Jump to bottom

implement billing banner #43267

Merged
merged 8 commits into from
Jun 11, 2024
Merged

implement billing banner #43267

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bdd0adc
implement billing banner
pasyukevich Jun 7, 2024
58fe381
fix header
pasyukevich Jun 7, 2024
2487592
fix banner subtitle
pasyukevich Jun 7, 2024
94f0a01
remove specific style
pasyukevich Jun 10, 2024
f0e23c9
remove specific lineHeight
pasyukevich Jun 10, 2024
0dd74d7
update props, add variables
pasyukevich Jun 10, 2024
5ffec6f
update ui
pasyukevich Jun 11, 2024
1baaae0
remove dummy code
pasyukevich Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions assets/images/simple-illustrations/simple-illustration__creditcardeyes.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 2 additions & 0 deletions src/components/Icon/Illustrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import CompanyCard from '@assets/images/simple-illustrations/simple-illustration
import ConciergeBubble from '@assets/images/simple-illustrations/simple-illustration__concierge-bubble.svg';
import ConciergeNew from '@assets/images/simple-illustrations/simple-illustration__concierge.svg';
import CreditCardsNew from '@assets/images/simple-illustrations/simple-illustration__credit-cards.svg';
import CreditCardEyes from '@assets/images/simple-illustrations/simple-illustration__creditcardeyes.svg';
import EmailAddress from '@assets/images/simple-illustrations/simple-illustration__email-address.svg';
import FolderOpen from '@assets/images/simple-illustrations/simple-illustration__folder-open.svg';
import Gears from '@assets/images/simple-illustrations/simple-illustration__gears.svg';
Expand Down Expand Up @@ -190,4 +191,5 @@ export {
ExpensifyApprovedLogoLight,
SendMoney,
CheckmarkCircle,
CreditCardEyes,
};
5 changes: 5 additions & 0 deletions src/components/Section/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ type SectionProps = Partial<ChildrenProps> & {

/** The height of the icon. */
iconHeight?: number;

/** Banner to display at the top of the section */
banner?: ReactNode;
};

function isIllustrationLottieAnimation(illustration: DotLottieAnimation | IconAsset | undefined): illustration is DotLottieAnimation {
Expand Down Expand Up @@ -119,6 +122,7 @@ function Section({
iconWidth,
iconHeight,
renderSubtitle,
banner = null,
}: SectionProps) {
const styles = useThemeStyles();
const theme = useTheme();
Expand All @@ -133,6 +137,7 @@ function Section({

return (
<View style={[styles.pageWrapper, styles.cardSectionContainer, containerStyles, (isCentralPane || !!illustration) && styles.p0]}>
{banner}
{cardLayout === CARD_LAYOUT.ICON_ON_TOP && (
<IconSection
width={iconWidth}
Expand Down
44 changes: 44 additions & 0 deletions src/pages/settings/Subscription/CardSection/BillingBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React from 'react';
import {View} from 'react-native';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import * as Illustrations from '@components/Icon/Illustrations';
import Text from '@components/Text';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';

type BillingBannerProps = {
title: string;
subtitle: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to make this optional? Are we guaranteed to have subtitle all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think they both should be optional (for example: we do not have titles for API errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

isError?: boolean;
shouldShowRedDotIndicator?: boolean;
};

function BillingBanner({title, subtitle, isError, shouldShowRedDotIndicator}: BillingBannerProps) {
const styles = useThemeStyles();
const theme = useTheme();

return (
<View style={[styles.pt4, styles.pb3, styles.ph5, styles.flexRow, styles.gap3, styles.w100, styles.alignItemsCenter, styles.hoveredComponentBG]}>
<Icon
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Expensify/design Would we have a case where we might not want to show any icon?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's confirm with @dannymcclain for this one too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently do not have any cases where we don't have an icon. And playing around with it in Figma, the icon really contributes a lot to the layout so I'm inclined to say we should always include one.

src={isError ? Illustrations.CreditCardEyes : Illustrations.CheckmarkCircle}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the icon should be passed as a prop. Because we're planning to reuse this for RBR from other API failures like change of subscription plan

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or may be I am getting confused. This banner is specific to the Card section? and the other RBRs will be handled as different implementation?

Copy link
Contributor Author

@pasyukevich pasyukevich Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to pass here icon as a prop, for BRB pattern we will pass the shouldShowRedDotIndicator prop (it also displays on a different side)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, currently, this banner is specific

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant for all the errors it'll always show CreditCardEyes. Wouldn't we want to make this reusable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can update this to be more reusable and make it less specific in the future when it would be needed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with that.

width={48}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reuse existing variable sizes for example variables.menuIconSize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated with reuse

height={48}
/>
<View style={[styles.flex1, styles.justifyContentCenter]}>
<Text style={[styles.headerText, styles.textLarge]}>{title}</Text>
<Text style={[styles.textSupporting, styles.textLineHeightXLarge]}>{subtitle}</Text>
</View>
{isError && shouldShowRedDotIndicator && (
<Icon
src={Expensicons.DotIndicator}
fill={theme.danger}
/>
)}
</View>
);
}

BillingBanner.displayName = 'BillingBanner';

export default BillingBanner;
4 changes: 4 additions & 0 deletions src/styles/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,10 @@ const styles = (theme: ThemeColors) =>
fontWeight: FontUtils.fontWeight.bold,
},

textLineHeightXLarge: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lineHeight: variables.lineHeightXLarge,
},

fontWeightNormal: {
fontWeight: FontUtils.fontWeight.normal,
},
Expand Down
Loading