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.

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

(PC-33060) fix(android): multiple log consult offer events #7642

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion __mocks__/@react-navigation/native.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export const createNavigationContainerRef = () => ({
current: navigation,
})

export const useIsFocused = jest.fn()
export const useIsFocused = jest.fn().mockReturnValue(true)
export const useRoute = jest.fn().mockReturnValue({ params: {} })
export const useFocusEffect = useEffect
export const NavigationContainer = jest.fn()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as reactNavigationNative from '@react-navigation/native'
import React, { ComponentProps } from 'react'
import { Share } from 'react-native'

Expand All @@ -23,10 +24,10 @@ jest.mock('libs/firebase/remoteConfig/remoteConfig.services', () => ({
jest.mock('libs/network/NetInfoWrapper')

const mockNavigate = jest.fn()
jest.mock('@react-navigation/native', () => ({
...jest.requireActual('@react-navigation/native'),
useNavigation: () => ({ navigate: mockNavigate, push: jest.fn() }),
}))
jest.spyOn(reactNavigationNative, 'useNavigation').mockReturnValue({
navigate: mockNavigate,
push: jest.fn(),
})

jest.mock('libs/subcategories/useSubcategory')

Expand Down
1 change: 0 additions & 1 deletion src/features/navigation/TabBar/TabBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ export const TabBar: React.FC<Props> = ({ navigation, state }) => {
return (
<TabBarComponent
navigateTo={{ screen: tabNavConfig[0], params: tabNavConfig[1] }}
enableNavigate={false}
key={`key-tab-nav-${route.key}`}
tabName={route.name}
isSelected={route.isSelected}
Expand Down
14 changes: 5 additions & 9 deletions src/features/navigation/TabBar/TabBarContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react'
import styled from 'styled-components/native'

import { useNetInfoContext } from 'libs/network/NetInfoWrapper'
import { getShadow, getSpacing, Spacer } from 'ui/theme'
import { getShadow, getSpacing } from 'ui/theme'

import { useCustomSafeInsets } from '../../../ui/theme/useCustomSafeInsets'

Expand All @@ -11,32 +11,28 @@ export const TabBarContainer = ({ children }: { children: React.ReactNode }) =>
const netInfo = useNetInfoContext()
return (
<MainContainer>
<RowContainer>
<Spacer.Row numberOfSpaces={4} />
{children}
<Spacer.Row numberOfSpaces={4} />
</RowContainer>
<RowContainer>{children}</RowContainer>
<SafeAreaPlaceholder safeHeight={netInfo.isConnected ? bottom : 0} />
</MainContainer>
)
}

const RowContainer = styled.View({
flexDirection: 'row',
width: '100%',
justifyContent: 'space-evenly',
paddingHorizontal: getSpacing(4),
})

const SafeAreaPlaceholder = styled.View<{ safeHeight: number }>(({ safeHeight }) => ({
height: safeHeight,
}))

const MainContainer = styled.View(({ theme }) => ({
alignItems: 'center',
borderTopStyle: 'solid',
borderTopWidth: getSpacing(1 / 4),
borderTopColor: theme.colors.greyMedium,
backgroundColor: theme.uniqueColors.tabBar,
width: '100%',
backgroundColor: theme.uniqueColors.tabBar,
position: 'absolute',
bottom: 0,
...getShadow({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as reactNavigationNative from '@react-navigation/native'
import React from 'react'

import { chroniclesSnap } from 'features/chronicle/fixtures/chroniclesSnap'
Expand All @@ -6,10 +7,10 @@ import { render, screen, userEvent } from 'tests/utils'
import { ChronicleSection } from './ChronicleSection'

const mockNavigate = jest.fn()
jest.mock('@react-navigation/native', () => ({
...jest.requireActual('@react-navigation/native'),
useNavigation: () => ({ navigate: mockNavigate, push: jest.fn() }),
}))
jest.spyOn(reactNavigationNative, 'useNavigation').mockReturnValue({
navigate: mockNavigate,
push: jest.fn(),
})

describe('ChroniclesSection', () => {
const user = userEvent.setup()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as reactNavigationNative from '@react-navigation/native'
import React from 'react'

import { chroniclesSnap } from 'features/chronicle/fixtures/chroniclesSnap'
Expand All @@ -6,12 +7,12 @@ import { fireEvent, render, screen } from 'tests/utils/web'
import { ChronicleSection } from './ChronicleSection'

const mockNavigate = jest.fn()
jest.mock('@react-navigation/native', () => ({
...jest.requireActual('@react-navigation/native'),
useNavigation: () => ({ navigate: mockNavigate, push: jest.fn() }),
}))
jest.spyOn(reactNavigationNative, 'useNavigation').mockReturnValue({
navigate: mockNavigate,
push: jest.fn(),
})

describe('ChroniclesSection', () => {
describe('ChronicleSection', () => {
it('should render correctly in mobile', () => {
render(
<ChronicleSection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ jest.mock('@react-navigation/native', () => ({
...jest.requireActual('@react-navigation/native'),
useNavigation: () => ({ navigate: mockNavigate, push: jest.fn() }),
useFocusEffect: jest.fn(),
useIsFocused: () => true,
}))

jest.mock('features/venueMap/useGetAllVenues')
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import * as reactNavigationNative from '@react-navigation/native'
import React from 'react'
import { Text } from 'react-native'

import { navigate, push } from '__mocks__/@react-navigation/native'
import { navigateFromRef, pushFromRef } from 'features/navigation/navigationRef'
import { render, screen, userEvent } from 'tests/utils'
import { InternalTouchableLink } from 'ui/components/touchableLink/InternalTouchableLink'

jest.mock('features/navigation/navigationRef')

const mockPush = jest.fn()
const mockNavigate = jest.fn()
jest.spyOn(reactNavigationNative, 'useNavigation').mockReturnValue({
navigate: mockNavigate,
push: mockPush,
})

const navigateToItineraryMock = jest.fn()
const useItinerary = () => ({
navigateTo: navigateToItineraryMock,
Expand Down Expand Up @@ -39,7 +46,7 @@ describe('<InternalTouchableLink />', () => {

await user.press(screen.getByText(linkText))

expect(navigate).toHaveBeenCalledWith('TabNavigator', { screen: 'Home', params: undefined })
expect(mockNavigate).toHaveBeenCalledWith('TabNavigator', { screen: 'Home', params: undefined })
})

it('should push right screen with expected params if withPush={true}', async () => {
Expand All @@ -52,13 +59,13 @@ describe('<InternalTouchableLink />', () => {

await user.press(screen.getByText(linkText))

expect(push).toHaveBeenCalledWith('TabNavigator', {
expect(mockPush).toHaveBeenCalledWith('TabNavigator', {
screen: 'Home',
params: undefined,
})
})

it('should push screen only once in case of press spamming when withPush={true}', async () => {
it('should push screen only once in case of press spamming', async () => {
render(
<InternalTouchableLink
navigateTo={{ screen: 'TabNavigator', params: { screen: 'Home' }, withPush: true }}>
Expand All @@ -70,7 +77,7 @@ describe('<InternalTouchableLink />', () => {
await user.press(screen.getByText(linkText))
await user.press(screen.getByText(linkText))

expect(push).toHaveBeenNthCalledWith(1, 'TabNavigator', {
expect(mockPush).toHaveBeenNthCalledWith(1, 'TabNavigator', {
screen: 'Home',
params: undefined,
})
Expand Down Expand Up @@ -129,7 +136,7 @@ describe('<InternalTouchableLink />', () => {

await user.press(screen.getByText(linkText))

expect(navigate).not.toHaveBeenCalledWith('TabNavigator', {
expect(mockNavigate).not.toHaveBeenCalledWith('TabNavigator', {
screen: 'Home',
params: undefined,
})
Expand Down
34 changes: 17 additions & 17 deletions src/ui/components/touchableLink/InternalTouchableLink.tsx
Original file line number Diff line number Diff line change
@@ -1,43 +1,43 @@
import { useLinkProps, useNavigation } from '@react-navigation/native'
import { useIsFocused, useLinkProps, useNavigation } from '@react-navigation/native'
import React, { FunctionComponent, useCallback } from 'react'

import { pushFromRef, navigateFromRef, resetFromRef } from 'features/navigation/navigationRef'
import { UseNavigationType } from 'features/navigation/RootNavigator/types'
import { TouchableLink } from 'ui/components/touchableLink/TouchableLink'
import { InternalTouchableLinkProps } from 'ui/components/touchableLink/types'

const PUSH_LINK_COOLDOWN = 1500

export const InternalTouchableLink: FunctionComponent<InternalTouchableLinkProps> = ({
navigateTo,
enableNavigate = true,
onBeforeNavigate,
onAfterNavigate,
...rest
}) => {
// We use nullish operator here because TabBar uses InternalTouchableLink but navigateTo is undefined during launch
const internalLinkProps = useLinkProps({ to: navigateTo ?? '' })
const isFocused = useIsFocused()
const { navigate, push, reset } = useNavigation<UseNavigationType>()
const { screen, params, fromRef, withPush, withReset } = navigateTo

// To avoid double tap issue, we define navigation functions only when component is focused
const handleNavigation = useCallback(() => {
if (enableNavigate) {
if (withReset) {
fromRef
? resetFromRef(screen, params)
: reset({ index: 0, routes: [{ name: screen, params }] })
} else if (withPush) {
fromRef ? pushFromRef(screen, params) : push(screen, params)
} else {
fromRef ? navigateFromRef(screen, params) : navigate(screen, params)
}
if (withReset) {
fromRef
? resetFromRef(screen, params)
: reset({ index: 0, routes: [{ name: screen, params }] })
} else if (withPush) {
fromRef ? pushFromRef(screen, params) : push(screen, params)
} else {
fromRef ? navigateFromRef(screen, params) : navigate(screen, params)
}
}, [enableNavigate, navigate, push, reset, fromRef, withPush, withReset, params, screen])
// For link in push mode, we want to avoid double tap. So we put a pretty large cooldown wait value
}, [navigate, push, reset, fromRef, withPush, withReset, params, screen])
return (
<TouchableLink
handleNavigation={handleNavigation}
handleNavigation={enableNavigate && isFocused ? handleNavigation : undefined}
onBeforeNavigate={isFocused ? onBeforeNavigate : undefined}
onAfterNavigate={isFocused ? onAfterNavigate : undefined}
linkProps={internalLinkProps}
{...rest}
pressCooldownDelay={withPush ? PUSH_LINK_COOLDOWN : undefined}
/>
)
}
53 changes: 1 addition & 52 deletions src/ui/components/touchableLink/TouchableLink.native.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react'
import { Text } from 'react-native'

import { analytics } from 'libs/analytics/provider'
import { act, render, screen, userEvent } from 'tests/utils'
import { render, screen, userEvent } from 'tests/utils'
import { ButtonPrimary } from 'ui/components/buttons/ButtonPrimary'

import { TouchableLink } from './TouchableLink'
Expand Down Expand Up @@ -73,57 +73,6 @@ describe('<TouchableLink />', () => {
expect(link.props.style).toEqual(expectedStyle)
})

it('should trigger handleNavigation only once in case of press spamming with correct cooldown delay', async () => {
render(
<TouchableLink handleNavigation={handleNavigationMock} pressCooldownDelay={300}>
<TouchableLinkContent />
</TouchableLink>
)

await user.press(screen.getByText(linkText))
await user.press(screen.getByText(linkText))
await user.press(screen.getByText(linkText))

expect(handleNavigationMock).toHaveBeenCalledTimes(1)
})

it('should trigger handleNavigation multiple time if cooldown delay is respected', async () => {
render(
<TouchableLink handleNavigation={handleNavigationMock} pressCooldownDelay={300}>
<TouchableLinkContent />
</TouchableLink>
)

await act(async () => {
await user.press(screen.getByText(linkText))

jest.advanceTimersByTime(300)

await user.press(screen.getByText(linkText))

jest.advanceTimersByTime(300)

await user.press(screen.getByText(linkText))
})

expect(handleNavigationMock).toHaveBeenCalledTimes(3)
})

it('should trigger handleNavigation with multiple press when no cooldown delay is set', async () => {
jest.useFakeTimers()
render(
<TouchableLink handleNavigation={handleNavigationMock}>
<TouchableLinkContent />
</TouchableLink>
)

await user.press(screen.getByText(linkText))
await user.press(screen.getByText(linkText))
await user.press(screen.getByText(linkText))

expect(handleNavigationMock).toHaveBeenCalledTimes(3)
})

it('should not trigger handleNavigation when Link is disabled', async () => {
render(
<TouchableLink handleNavigation={handleNavigationMock} disabled>
Expand Down
11 changes: 2 additions & 9 deletions src/ui/components/touchableLink/TouchableLink.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { throttle } from 'lodash'
import React, { createRef, ElementType, useCallback, useEffect, useMemo } from 'react'
import { NativeSyntheticEvent, Platform, TargetedEvent } from 'react-native'
import styled from 'styled-components/native'
Expand Down Expand Up @@ -28,7 +27,6 @@ export function TouchableLink({
hoverUnderlineColor,
accessibilityLabel,
testID,
pressCooldownDelay = 0,
...rest
}: TouchableLinkProps) {
const TouchableComponent = (
Expand Down Expand Up @@ -73,7 +71,7 @@ export function TouchableLink({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [])

const pressFn = useMemo(
const handlePress = useMemo(
() =>
handleNavigationWrapper({
onBeforeNavigate,
Expand All @@ -83,11 +81,6 @@ export function TouchableLink({
[onBeforeNavigate, onAfterNavigate, handleNavigation]
)

const throttledPressFn = useMemo(
() => throttle(pressFn, pressCooldownDelay, { leading: true, trailing: false }),
[pressCooldownDelay, pressFn]
)

return (
<TouchableLinkComponent
{...touchableOpacityProps}
Expand All @@ -100,7 +93,7 @@ export function TouchableLink({
onBlur={onLinkBlur}
onMouseEnter={onMouseEnter}
onMouseLeave={onMouseLeave}
onPress={disabled ? undefined : throttledPressFn}
onPress={disabled ? undefined : handlePress}
{...accessibilityAndTestId(accessibilityLabel, testID)}>
{children}
</TouchableLinkComponent>
Expand Down
2 changes: 1 addition & 1 deletion src/ui/components/touchableLink/handleNavigationWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const handleNavigationWrapper = ({
}: HandleNavigationWrapperProps) => {
const onClick = async (event: GestureResponderEvent) => {
if (onBeforeNavigate) await onBeforeNavigate(event)
handleNavigation()
handleNavigation?.()
if (onAfterNavigate) await onAfterNavigate(event)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const handleNavigationWrapper = ({
event.preventDefault()

if (onBeforeNavigate) await onBeforeNavigate(event)
handleNavigation()
handleNavigation?.()
if (onAfterNavigate) await onAfterNavigate(event)
}

Expand Down
Loading
Loading