Created a template for the secondary user product card in account overview#1671
Created a template for the secondary user product card in account overview#1671enesihsa wants to merge 3 commits into
Conversation
| </div> | ||
| {groupedProductType.tierLabel && ( | ||
| {isSecondary && | ||
| primarySubscriber && ( // TODO Separate more properly. |
There was a problem hiding this comment.
This is the main addition, makes it look like there is plenty of changes but not really, much is indentation. This conditional section renders the shared subscription details and leave subscription Card.Sections. Currently combined into a single one but there probably is a better way to do this?
| </Card.Section> | ||
| </> | ||
| )} | ||
| {!isSecondary && ( |
There was a problem hiding this comment.
This also causes many indentation changes.
There was a problem hiding this comment.
!isSecondary just renders what we had before right?
There was a problem hiding this comment.
Yes, I think by default we show billing information and payment method since all other plans require some form of payment, but this is the only one that doesn't show these.
| {cardConfig.getBenefitsSectionCopy && nextPaymentDetails && ( | ||
| <Card.Section backgroundColor="#edf5fA" removeBorders> | ||
| <p css={benefitsTextCss}> | ||
| {cardConfig.getBenefitsSectionCopy( | ||
| nextPaymentDetails, | ||
| )} | ||
| {isSecondary && primarySubscriber // TODO should probably find a better way to do this too. | ||
| ? `You're enjoying ${mainPlan.name} as part of a shared subscription.` | ||
| : cardConfig.getBenefitsSectionCopy( | ||
| nextPaymentDetails, | ||
| )} | ||
| </p> |
There was a problem hiding this comment.
It's possible that nextPaymentDetails will not exist, so
cardConfig.getBenefitsSectionCopy && nextPaymentDetails
Will always test false. You could do something like:
cardConfig.getBenefitsSectionCopy && (nextPaymentDetails || isSecondary) && (
And then
isSecondary ? "you are enjoying copy" : cardConfig.getBenefitsSectionCopy()
There was a problem hiding this comment.
Ah thanks, missed this one. Another question I had was would it be possible to have the secondary user details in the getBenefitsSectionCopy too (or a more generic way of fetching them), currently it has one for each Zuora subscription type I think, but there are exceptions now for secondary users and GW Gift subscriptions.
| </div> | ||
| {groupedProductType.tierLabel && ( | ||
| {isSecondary && | ||
| primarySubscriber && ( // TODO Separate more properly. |
There was a problem hiding this comment.
out of curiosity, why is the primarySubscriber needed?
There was a problem hiding this comment.
The product card will show the details (full name and email) of the associated primary user according to the figma models. In practice they preferably should come together (ie if the user is secondary then I assume backend should always return the primary user's details too?)
I should combine the primarySubscriber into determining isSecondary value probably unless if there's a scenario where we may have one but not the other.
There was a problem hiding this comment.
Ah ok, primarySubscriber is the owner of the original plan. That makes sense 👍 Oka, yeah you can put them together somewhere, as you are doing this check a couple of times across the file
j-ruda-guardian
left a comment
j-ruda-guardian
left a comment
There was a problem hiding this comment.
I think this looks good. May I ask you for a change that will make reviewing this files a lot easier:
Please, take the new secondary user card and extract it to a new file (This ProductCard file is already more than crowded enough) and then import it and render it conditionally like you are doing. That might solve this indentation nightmare and will also make the whole thing easier to review!
nice work, thank you!
enesihsa
force-pushed
the
ea/multiple-accounts/secondary-user-account-overview
branch
from
044e650 to
a756302
Compare
enesihsa
force-pushed
the
ea/multiple-accounts/secondary-user-account-overview
branch
from
deb41d5 to
a756302
Compare
enesihsa
force-pushed
the
ea/multiple-accounts/secondary-user-account-overview
branch
from
16d4f8c to
0567ca5
Compare
| /> | ||
| <JoinDateRow | ||
| }) => | ||
| !primaryUser && ( |
There was a problem hiding this comment.
I am really confused as to why this keeps causing the same issue. At least it's much more manageable here and easier to verify that nothing actually changes apart from the addition of this condition.
2 participants
TODO
Current situation/background
What does this PR change?
Next steps/further info