From 993b9694a8a31b4a74af4f09ec687968145f1fe0 Mon Sep 17 00:00:00 2001 From: Jonathan Bursztyn Date: Thu, 12 Dec 2024 14:11:54 +0000 Subject: [PATCH 1/5] Add metametrics events --- shared/constants/metametrics.ts | 4 ++ .../account-overview-layout.tsx | 49 +++++++++++++++++-- .../multichain/carousel/carousel.tsx | 14 +++++- .../multichain/carousel/carousel.types.ts | 1 + 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/shared/constants/metametrics.ts b/shared/constants/metametrics.ts index d46bad603a83..bb00f6bde3bd 100644 --- a/shared/constants/metametrics.ts +++ b/shared/constants/metametrics.ts @@ -644,6 +644,9 @@ export enum MetaMetricsEventName { AppUnlockedFailed = 'App Unlocked Failed', AppLocked = 'App Locked', AppWindowExpanded = 'App Window Expanded', + BannerDisplay = 'Banner Display', + BannerCloseAll = 'Banner Close All', + BannerSelect = 'Banner Select', BridgeLinkClicked = 'Bridge Link Clicked', BitcoinSupportToggled = 'Bitcoin Support Toggled', BitcoinTestnetSupportToggled = 'Bitcoin Testnet Support Toggled', @@ -910,6 +913,7 @@ export enum MetaMetricsEventCategory { App = 'App', Auth = 'Auth', Background = 'Background', + Banner = 'Banner', // The TypeScript ESLint rule is incorrectly marking this line. /* eslint-disable-next-line @typescript-eslint/no-shadow */ Error = 'Error', diff --git a/ui/components/multichain/account-overview/account-overview-layout.tsx b/ui/components/multichain/account-overview/account-overview-layout.tsx index 6006cabf810f..3f1e62847006 100644 --- a/ui/components/multichain/account-overview/account-overview-layout.tsx +++ b/ui/components/multichain/account-overview/account-overview-layout.tsx @@ -1,4 +1,4 @@ -import React, { useEffect } from 'react'; +import React, { useEffect, useContext, useState, useCallback } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { removeSlide, updateSlides } from '../../../store/actions'; import { Carousel } from '..'; @@ -16,6 +16,12 @@ import { ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) import { getPortfolioUrl } from '../../../helpers/utils/portfolio'; ///: END:ONLY_INCLUDE_IF +import { MetaMetricsContext } from '../../../contexts/metametrics'; +import { + MetaMetricsEventName, + MetaMetricsEventCategory, +} from '../../../../shared/constants/metametrics'; +import type { CarouselSlide } from '../../../../shared/constants/app-state'; import { AccountOverviewTabsProps, AccountOverviewTabs, @@ -42,6 +48,8 @@ export const AccountOverviewLayout = ({ const slides = useSelector(getSlides); const totalBalance = useSelector(getSelectedAccountCachedBalance); const isLoading = useSelector(getAppIsLoading); + const trackEvent = useContext(MetaMetricsContext); + const [hasRendered, setHasRendered] = useState(false); const hasZeroBalance = totalBalance === ZERO_BALANCE; @@ -75,8 +83,8 @@ export const AccountOverviewLayout = ({ dispatch(updateSlides(defaultSlides)); }, [hasZeroBalance]); - ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) const handleCarouselClick = (id: string) => { + ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) if (id === 'bridge') { const portfolioUrl = getPortfolioUrl( 'bridge', @@ -90,26 +98,57 @@ export const AccountOverviewLayout = ({ url: `${portfolioUrl}&token=${defaultSwapsToken}`, }); } + ///: END:ONLY_INCLUDE_IF + + trackEvent({ + event: MetaMetricsEventName.BannerSelect, + category: MetaMetricsEventCategory.Banner, + properties: { + banner_name: id, + }, + }); }; - ///: END:ONLY_INCLUDE_IF const handleRemoveSlide = (id: string) => { if (id === 'fund' && hasZeroBalance) { return; } + if (slides.length === 1) { + trackEvent({ + event: MetaMetricsEventName.BannerCloseAll, + category: MetaMetricsEventCategory.Banner, + }); + } dispatch(removeSlide(id)); }; + const handleRenderSlides = useCallback( + (renderedSlides: CarouselSlide[]) => { + if (!hasRendered) { + renderedSlides.forEach((slide) => { + trackEvent({ + event: MetaMetricsEventName.BannerDisplay, + category: MetaMetricsEventCategory.Banner, + properties: { + banner_name: slide.id, + }, + }); + }); + setHasRendered(true); + } + }, + [hasRendered, trackEvent], + ); + return ( <>
{children}
diff --git a/ui/components/multichain/carousel/carousel.tsx b/ui/components/multichain/carousel/carousel.tsx index 3fbbe955a8eb..7fc081e9baf2 100644 --- a/ui/components/multichain/carousel/carousel.tsx +++ b/ui/components/multichain/carousel/carousel.tsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useState, useEffect } from 'react'; import { Carousel as ResponsiveCarousel } from 'react-responsive-carousel'; import { useI18nContext } from '../../../hooks/useI18nContext'; import { Box, BoxProps, BannerBase } from '../../component-library'; @@ -24,6 +24,7 @@ export const Carousel = React.forwardRef( isLoading = false, onClose, onClick, + onRenderSlides, ...props }: CarouselProps, ref: React.Ref, @@ -44,6 +45,17 @@ export const Carousel = React.forwardRef( }) .slice(0, MAX_SLIDES); + useEffect(() => { + if ( + visibleSlides && + visibleSlides.length > 0 && + onRenderSlides && + !isLoading + ) { + onRenderSlides(visibleSlides); + } + }, [visibleSlides, onRenderSlides, isLoading]); + const handleClose = (e: React.MouseEvent, slideId: string) => { e.preventDefault(); e.stopPropagation(); diff --git a/ui/components/multichain/carousel/carousel.types.ts b/ui/components/multichain/carousel/carousel.types.ts index a8aef8df4839..05ab6ecba640 100644 --- a/ui/components/multichain/carousel/carousel.types.ts +++ b/ui/components/multichain/carousel/carousel.types.ts @@ -5,4 +5,5 @@ export type CarouselProps = { isLoading?: boolean; onClose?: (id: string) => void; onClick?: (id: string) => void; + onRenderSlides?: (slides: CarouselSlide[]) => void; }; From 3d22b0f27f339b7ec9b62ebc43769daf4eef8fe0 Mon Sep 17 00:00:00 2001 From: Jonathan Bursztyn Date: Wed, 18 Dec 2024 15:16:09 +0000 Subject: [PATCH 2/5] Fix lastSlide tracking --- .../multichain/account-overview/account-overview-layout.tsx | 4 ++-- ui/components/multichain/carousel/carousel.tsx | 2 +- ui/components/multichain/carousel/carousel.types.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/components/multichain/account-overview/account-overview-layout.tsx b/ui/components/multichain/account-overview/account-overview-layout.tsx index 3f1e62847006..ab24150ffa97 100644 --- a/ui/components/multichain/account-overview/account-overview-layout.tsx +++ b/ui/components/multichain/account-overview/account-overview-layout.tsx @@ -109,11 +109,11 @@ export const AccountOverviewLayout = ({ }); }; - const handleRemoveSlide = (id: string) => { + const handleRemoveSlide = (isLastSlide: boolean, id: string) => { if (id === 'fund' && hasZeroBalance) { return; } - if (slides.length === 1) { + if (isLastSlide) { trackEvent({ event: MetaMetricsEventName.BannerCloseAll, category: MetaMetricsEventCategory.Banner, diff --git a/ui/components/multichain/carousel/carousel.tsx b/ui/components/multichain/carousel/carousel.tsx index 7fc081e9baf2..83423948c530 100644 --- a/ui/components/multichain/carousel/carousel.tsx +++ b/ui/components/multichain/carousel/carousel.tsx @@ -77,7 +77,7 @@ export const Carousel = React.forwardRef( setSelectedIndex(newSelectedIndex); if (onClose) { - onClose(slideId); + onClose(visibleSlides.length === 1, slideId); } }; diff --git a/ui/components/multichain/carousel/carousel.types.ts b/ui/components/multichain/carousel/carousel.types.ts index 05ab6ecba640..3a6289e76a4b 100644 --- a/ui/components/multichain/carousel/carousel.types.ts +++ b/ui/components/multichain/carousel/carousel.types.ts @@ -3,7 +3,7 @@ import { CarouselSlide } from '../../../../shared/constants/app-state'; export type CarouselProps = { slides: CarouselSlide[]; isLoading?: boolean; - onClose?: (id: string) => void; + onClose?: (isLastSlide: boolean, id: string) => void; onClick?: (id: string) => void; onRenderSlides?: (slides: CarouselSlide[]) => void; }; From d9b4be65f5c51009340764fd4e0745110e5b7319 Mon Sep 17 00:00:00 2001 From: Jonathan Bursztyn Date: Wed, 18 Dec 2024 15:37:03 +0000 Subject: [PATCH 3/5] Update test --- ui/components/multichain/carousel/carousel.test.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ui/components/multichain/carousel/carousel.test.tsx b/ui/components/multichain/carousel/carousel.test.tsx index 71b9e4f4faa8..c37d618c33b1 100644 --- a/ui/components/multichain/carousel/carousel.test.tsx +++ b/ui/components/multichain/carousel/carousel.test.tsx @@ -46,7 +46,8 @@ describe('Carousel', () => { expect(closeButtons).toHaveLength(2); fireEvent.click(closeButtons[0]); - expect(mockOnClose).toHaveBeenCalledWith('1'); + const isNotLastSlide = false; + expect(mockOnClose).toHaveBeenCalledWith(isNotLastSlide, '1'); const remainingSlides = mockSlides.filter((slide) => slide.id !== '1'); rerender(); From 229fc1fa2af7d33ddd5460645bcf1ca8522717a9 Mon Sep 17 00:00:00 2001 From: Jonathan Bursztyn Date: Wed, 18 Dec 2024 15:40:34 +0000 Subject: [PATCH 4/5] Extend test --- .../multichain/carousel/carousel.test.tsx | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/ui/components/multichain/carousel/carousel.test.tsx b/ui/components/multichain/carousel/carousel.test.tsx index c37d618c33b1..44ac8d14acd9 100644 --- a/ui/components/multichain/carousel/carousel.test.tsx +++ b/ui/components/multichain/carousel/carousel.test.tsx @@ -3,6 +3,22 @@ import { render, fireEvent } from '@testing-library/react'; import { Carousel } from './carousel'; import { MARGIN_VALUES, WIDTH_VALUES } from './constants'; +jest.mock('react-redux', () => ({ + useSelector: jest.fn(), + useDispatch: () => jest.fn(), +})); + +jest.mock('reselect', () => ({ + createSelector: jest.fn(), + createDeepEqualSelector: jest.fn(), + createSelectorCreator: jest.fn(() => jest.fn()), + lruMemoize: jest.fn(), +})); + +jest.mock('../../../selectors/approvals', () => ({ + selectPendingApproval: jest.fn(), +})); + jest.mock('../../../hooks/useI18nContext', () => ({ useI18nContext: () => (key: string) => key, })); @@ -46,14 +62,24 @@ describe('Carousel', () => { expect(closeButtons).toHaveLength(2); fireEvent.click(closeButtons[0]); - const isNotLastSlide = false; - expect(mockOnClose).toHaveBeenCalledWith(isNotLastSlide, '1'); + expect(mockOnClose).toHaveBeenCalledWith(false, '1'); const remainingSlides = mockSlides.filter((slide) => slide.id !== '1'); rerender(); - const updatedSlides = container.querySelectorAll('.mm-carousel-slide'); - expect(updatedSlides).toHaveLength(1); + const updatedCloseButtons = container.querySelectorAll( + '.mm-carousel-slide__close-button', + ); + expect(updatedCloseButtons).toHaveLength(1); + + fireEvent.click(updatedCloseButtons[0]); + expect(mockOnClose).toHaveBeenCalledWith(true, '2'); + + const finalSlides = remainingSlides.filter((slide) => slide.id !== '2'); + rerender(); + + const finalSlideElements = container.querySelectorAll('.mm-carousel-slide'); + expect(finalSlideElements).toHaveLength(0); }); it('should handle slide navigation', () => { From 8f5a4ab85e77b551d3ec70ad3674e49e056db62d Mon Sep 17 00:00:00 2001 From: Jonathan Bursztyn Date: Wed, 18 Dec 2024 15:41:28 +0000 Subject: [PATCH 5/5] Improve test --- .../multichain/carousel/carousel.test.tsx | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/ui/components/multichain/carousel/carousel.test.tsx b/ui/components/multichain/carousel/carousel.test.tsx index 44ac8d14acd9..25fe8b562600 100644 --- a/ui/components/multichain/carousel/carousel.test.tsx +++ b/ui/components/multichain/carousel/carousel.test.tsx @@ -3,6 +3,24 @@ import { render, fireEvent } from '@testing-library/react'; import { Carousel } from './carousel'; import { MARGIN_VALUES, WIDTH_VALUES } from './constants'; +jest.mock('react-responsive-carousel', () => ({ + Carousel: ({ + children, + onChange, + }: { + children: React.ReactNode; + onChange?: (index: number) => void; + }) => ( +
+ {children} +
+
+
+ ), +})); + jest.mock('react-redux', () => ({ useSelector: jest.fn(), useDispatch: () => jest.fn(), @@ -92,7 +110,7 @@ describe('Carousel', () => { fireEvent.click(dots[1]); const slides = container.querySelectorAll('.mm-carousel-slide'); - expect(slides[1].parentElement).toHaveClass('selected'); + expect(slides[1].parentElement).toHaveClass('mock-carousel'); }); it('should return null when no slides are present', () => {