-
Notifications
You must be signed in to change notification settings - Fork 0
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
FB-324-332-website-changes-and-remove-FOUC #57
Conversation
…classNames to the buttons and change styling
src/sections/Pricing/Pricing.tsx
Outdated
@@ -26,64 +27,91 @@ const DescriptionComponent = styled.div` | |||
margin-bottom: 32px; | |||
`; | |||
|
|||
type PricingCard = { |
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.
Isn't it the same type as PricingCardProps
? But as I mentioned in a previous comment - I'm not sure we need it at all.
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.
Hm, for now it is but for example what if we will want to pass another prop to the PricingCard
component that is not related to this array? Also, this component and array are in different files and I am worried that it would be less readable if we will reuse this PricingCardProps
type. But maybe I am wrong about this?
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.
Okay, let's leave this array typed, but I'd change it a bit around.
For PricingCard.tsx
let's do the following:
export type PricingCardData = {
color: string;
buttonText: string;
buttonLink: string;
title: string;
price?: string;
denominator?: string;
isMostPopular?: boolean;
benefits: string[] | string;
btnClassName:
| 'btn-trial'
| 'btn-buy-monthly'
| 'btn-buy-yearly'
| 'btn-buy-lifelong'
| 'btn-buy-custom';
};
type PricingCardProps = PricingCardData;
- Let's add an additional check for
btnClassName
(as I understand it's for analytics reasons, so let's have it strictly typed) - Let's move the type of
PricingCardProps
toPricingCardData
(it will be easier to understand the import of this type inPricing.tsx
) - We can say the
PricingCardProps
isPricingCardData
. If we want to add additional props, we can just dotype PricingCardProps = { someOtherProp: string } & PricingCardData
.
Last of all we can get rid of PricingCard
type from Pricing.tsx
and import the one from PricingCard.tsx
-> we will have only a single source of type, I think it's much less confusing than having 2 different types saying basically the same.
What do you think about that?
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.
I super much like that idea! ❤️
This PR contains:
custom.css
file.