[LLD] - [LIVE-10488] - Add Portfolio Content Card #6032

sshmaxime · 2024-01-29T08:36:14Z

📝 Description

Remove old Carousel and use the new one
Remove extra unused files
Add Action Content Card
Add LLD portfolio carousel feature flags
Add usePortfolioCards hooks

NOTE:

We currently have some limitations on LLD (due to the fact that each variant has to be displayed in 2 different part of the DOM) that forced me to create a single component displayable in two different place for each variant, VariantA and VariantB.

❓ Context

JIRA or GitHub link: LIVE-10488

✅ Checklist

Pull Requests must pass the CI and be code reviewed. Set as Draft if the PR is not ready.

npx changeset was attached.
Covered by automatic tests.
[ ] Impact of the changes:

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-01-29T08:36:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-ui-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 6, 2024 2:38pm
web-tools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 6, 2024 2:38pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Feb 6, 2024 2:38pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Feb 6, 2024 2:38pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Feb 6, 2024 2:38pm

apps/ledger-live-desktop/src/renderer/components/ContentCards/ActionCard/components.tsx

apps/ledger-live-desktop/src/renderer/hooks/usePortfolioCards.tsx

apps/ledger-live-desktop/src/renderer/screens/dashboard/index.tsx

apps/ledger-live-desktop/src/renderer/screens/dashboard/PortfolioContentCards.tsx

KVNLS · 2024-02-01T15:23:24Z

apps/ledger-live-desktop/src/renderer/hooks/usePortfolioCards.tsx

+ // For some reason braze won't log the click event if the card url is empty
+ // Setting it as the card id just to have a dummy non empty value
+ // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+ // @ts-ignore


We need to ban // @ts-ignore OR any from our TS usage with new features.
Lately, we had an issue on production with a TS error that we ignored. We need to find why For some reason braze won't log the click or extend the type.

I 100% agree with you, there are a lot of "hidden" issues. This part of code isn't new, it's re-used code from the legacy piece of code that was handling braze before on LLD and left it as is.
I just dug a little and after few tests, all of the cards I receive from Braze have their url field set. So I'm just removing this part of code but again, I cannot fully know what will be the consequences of this code removal, hence the reason why I kept it in the first place.

FYI, on top that, the SDK we are using is almost 1 year old. We might need some time to get it up to date as well ..

KVNLS · 2024-02-01T15:36:41Z

apps/ledger-live-desktop/src/renderer/hooks/usePortfolioCards.tsx

+ const [cachedContentCards, setCachedContentCards] = useState<braze.Card[]>([]);
+ const portfolioCards = useSelector(portfolioContentCardSelector);
+
+ useEffect(() => {
+ const cards = braze.getCachedContentCards().cards;
+ setCachedContentCards(cards);
+ }, []);


Is there a reason why we don't initiate cachedContentCards with braze.getCachedContentCards().cards?
This would allow us to remove a useEffect

I re-used part of the old code and that was how it was done and I didn't give it a second thought. Agreed, nice catch, updating it right now.

KVNLS · 2024-02-01T15:47:19Z

apps/ledger-live-desktop/src/renderer/components/ContentCards/ActionCard/index.tsx

+ <Description>{description}</Description>
+ </Body>
+ <Actions>
+ <Link size="small" onClick={() => actions.dismiss.action()}>


Suggested change

<Link size="small" onClick={() => actions.dismiss.action()}>

<Link size="small" onClick={actions.dismiss.action}>

It's not the same type so I rather keep it that way. But we definitely should have a convention for this kind of things. Another thing to have in mind when re-thinking LLD or else.

KVNLS · 2024-02-01T15:47:36Z

apps/ledger-live-desktop/src/renderer/components/ContentCards/ActionCard/index.tsx

+ </Link>
+
+ {actions.primary.label && (
+ <ButtonV3 big variant="main" onClick={() => actions.primary.action()}>


Suggested change

<ButtonV3 big variant="main" onClick={() => actions.primary.action()}>

<ButtonV3 big variant="main" onClick={ actions.primary.action}>

KVNLS · 2024-02-01T15:59:16Z

Can you provide a way to test it, please?
A mock of portfolioCards or Braze setup with a campaign we can use in test.

cgrellard-ledger · 2024-02-01T16:54:16Z

Nice job Maxime ! 💪

KVNLS

When I have only one card in the Carousel I should not have the bottom bar:

Figma:
https://www.figma.com/file/nEFjjnCjeYCDwSENXf5RdY/Braze?type=design&node-id=4209-8820&mode=design&t=iTUtAeZw0QGrGH4g-4

KVNLS

Works well!
Please cleanup your commits if you create a merge commit, otherwise it's all good!

…for Portfolio ContentCards

… click on card

… card in Carousel

[LLD] - [LIVE-10488] - Add Portfolio Content Card

sshmaxime requested a review from a team as a code owner January 29, 2024 08:36

github-actions bot assigned sshmaxime Jan 29, 2024

live-github-bot bot added desktop Has changes in LLD common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs labels Jan 29, 2024

sshmaxime marked this pull request as draft January 29, 2024 09:57

sshmaxime force-pushed the feat/LIVE-10488 branch from ef8a42f to 693030b Compare January 29, 2024 14:31

live-github-bot bot added the mobile Has changes in LLM label Jan 30, 2024

Base automatically changed from feat/LIVE-10489 to develop January 30, 2024 16:14

sshmaxime force-pushed the feat/LIVE-10488 branch from 232a01a to 209779e Compare February 1, 2024 10:07

sshmaxime marked this pull request as ready for review February 1, 2024 10:08

mcayuelas-ledger requested changes Feb 1, 2024

View reviewed changes

mcayuelas-ledger self-requested a review February 1, 2024 15:04

mcayuelas-ledger previously approved these changes Feb 1, 2024

View reviewed changes

KVNLS requested changes Feb 1, 2024

View reviewed changes

cgrellard-ledger previously approved these changes Feb 1, 2024

View reviewed changes

sshmaxime dismissed stale reviews from cgrellard-ledger and mcayuelas-ledger via 2ff6a08 February 1, 2024 21:02

sshmaxime requested a review from KVNLS February 2, 2024 08:56

sshmaxime force-pushed the feat/LIVE-10488 branch from 5aa7799 to bc13d63 Compare February 5, 2024 10:21

sshmaxime requested a review from a team as a code owner February 5, 2024 12:10

KVNLS requested changes Feb 5, 2024

View reviewed changes

live-github-bot bot added the ui Has changes in the design system library label Feb 5, 2024

sshmaxime force-pushed the feat/LIVE-10488 branch from 0b87428 to 4f1cd07 Compare February 5, 2024 15:57

vercel bot deployed to Preview – react-ui-storybook February 5, 2024 16:06 View deployment

vercel bot deployed to Preview – web-tools February 5, 2024 16:08 View deployment

KVNLS previously approved these changes Feb 5, 2024

View reviewed changes

sshmaxime force-pushed the feat/LIVE-10488 branch from 4f1cd07 to 8290979 Compare February 5, 2024 16:44

vercel bot had a problem deploying to Preview – web-tools February 5, 2024 16:46 Failure

vercel bot deployed to Preview – react-ui-storybook February 5, 2024 16:46 View deployment

sshmaxime force-pushed the feat/LIVE-10488 branch from 8290979 to 3555290 Compare February 5, 2024 16:49

vercel bot deployed to Preview – web-tools February 5, 2024 16:50 View deployment

vercel bot deployed to Preview – react-ui-storybook February 5, 2024 16:51 View deployment

sshmaxime added 11 commits February 6, 2024 15:34

Setup ABTesting feature flag for the portfolio carousel

c770505

Remove old Carousel in LLD

ad09146

Add Action Card

7e805e1

Update braze types + update braze hook + add usePortfolioCards hooks

4766648

Add Variant A and B on Portfolio Carousel

cc55a8c

Rename LlmAnalyticsOptInPromptVariants to ABTestingVariants on LLM

5221cf7

Add onView on ContentCard + add VariantB and VariantA functionnality …

9e772de

…for Portfolio ContentCards

Avoid displaying Content Cards when there is no content cards

46eb3c3

Add Card wrapper around Carousel VariantA + remove custom checks when…

47281ea

… click on card

Disable braze when running Playwright accross LLD

d37bdfc

Unify design for Recover and Action cards + remove footer when only 1…

3ca1477

… card in Carousel

sshmaxime dismissed KVNLS’s stale review via 3ca1477 February 6, 2024 14:34

sshmaxime force-pushed the feat/LIVE-10488 branch from 3555290 to 3ca1477 Compare February 6, 2024 14:34

vercel bot deployed to Preview – react-ui-storybook February 6, 2024 14:36 View deployment

mcayuelas-ledger approved these changes Feb 6, 2024

View reviewed changes

vercel bot deployed to Preview – web-tools February 6, 2024 14:38 View deployment

sshmaxime merged commit 5c46443 into develop Feb 6, 2024
60 of 61 checks passed

sshmaxime deleted the feat/LIVE-10488 branch February 6, 2024 15:58

jdabbech-ledger pushed a commit that referenced this pull request Feb 19, 2024

Merge pull request #6032 from LedgerHQ/feat/LIVE-10488

db87454

[LLD] - [LIVE-10488] - Add Portfolio Content Card

jdabbech-ledger pushed a commit that referenced this pull request Feb 19, 2024

Merge pull request #6032 from LedgerHQ/feat/LIVE-10488

074e151

[LLD] - [LIVE-10488] - Add Portfolio Content Card

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLD] - [LIVE-10488] - Add Portfolio Content Card #6032

[LLD] - [LIVE-10488] - Add Portfolio Content Card #6032

sshmaxime commented Jan 29, 2024 •

edited

Loading

vercel bot commented Jan 29, 2024 •

edited

Loading

KVNLS Feb 1, 2024

sshmaxime Feb 1, 2024 •

edited

Loading

KVNLS Feb 1, 2024

sshmaxime Feb 1, 2024

KVNLS Feb 1, 2024

sshmaxime Feb 1, 2024 •

edited

Loading

KVNLS Feb 1, 2024

sshmaxime Feb 1, 2024

KVNLS commented Feb 1, 2024

cgrellard-ledger commented Feb 1, 2024

KVNLS left a comment

KVNLS left a comment •

edited

Loading

	<Link size="small" onClick={() => actions.dismiss.action()}>
	<Link size="small" onClick={actions.dismiss.action}>

	<ButtonV3 big variant="main" onClick={() => actions.primary.action()}>
	<ButtonV3 big variant="main" onClick={ actions.primary.action}>

[LLD] - [LIVE-10488] - Add Portfolio Content Card #6032

[LLD] - [LIVE-10488] - Add Portfolio Content Card #6032

Conversation

sshmaxime commented Jan 29, 2024 • edited Loading

📝 Description

NOTE:

❓ Context

✅ Checklist

[ ] Impact of the changes:

🧐 Checklist for the PR Reviewers

vercel bot commented Jan 29, 2024 • edited Loading

KVNLS Feb 1, 2024

Choose a reason for hiding this comment

sshmaxime Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

KVNLS Feb 1, 2024

Choose a reason for hiding this comment

sshmaxime Feb 1, 2024

Choose a reason for hiding this comment

KVNLS Feb 1, 2024

Choose a reason for hiding this comment

sshmaxime Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

KVNLS Feb 1, 2024

Choose a reason for hiding this comment

sshmaxime Feb 1, 2024

Choose a reason for hiding this comment

KVNLS commented Feb 1, 2024

cgrellard-ledger commented Feb 1, 2024

KVNLS left a comment

Choose a reason for hiding this comment

KVNLS left a comment • edited Loading

Choose a reason for hiding this comment

sshmaxime commented Jan 29, 2024 •

edited

Loading

vercel bot commented Jan 29, 2024 •

edited

Loading

sshmaxime Feb 1, 2024 •

edited

Loading

sshmaxime Feb 1, 2024 •

edited

Loading

KVNLS left a comment •

edited

Loading