Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,6 @@ function BaseTextInput({
// When autoGrowHeight is true we calculate the width for the text input, so it will break lines properly
// or if multiline is not supplied we calculate the text input height, using onLayout.
onLayout={onLayout}
accessibilityLabel={accessibilityLabel}
style={[
autoGrowHeight &&
!isAutoGrowHeightMarkdown &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ function BaseTextInput({
role={CONST.ROLE.PRESENTATION}
onPress={onPress}
tabIndex={-1}
accessibilityLabel={accessibilityLabel}
accessible={false}
// When autoGrowHeight is true we calculate the width for the text input, so it will break lines properly
// or if multiline is not supplied we calculate the text input height, using onLayout.
onLayout={onLayout}
Expand Down Expand Up @@ -488,7 +488,8 @@ function BaseTextInput({
readOnly={isReadOnly}
defaultValue={defaultValue}
markdownStyle={markdownStyle}
accessibilityLabel={inputProps.accessibilityLabel}
accessibilityLabel={inputProps.accessibilityLabel ?? accessibilityLabel}
keyboardType={inputProps.keyboardType}
aria-describedby={inputHelpText ? helpMessageId : undefined}
/>
{!!suffixCharacter && (
Expand Down
3 changes: 3 additions & 0 deletions src/components/TextInput/TextInputLabel/index.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ function TextInputLabel({label, labelScale, labelTranslateY, isMultiline}: TextI
return (
<Animated.View style={[styles.textInputLabelContainer, animatedStyle]}>
<Animated.Text
accessible={false}
accessibilityElementsHidden
importantForAccessibility="no"
numberOfLines={!isMultiline ? 1 : undefined}
ellipsizeMode={!isMultiline ? 'tail' : undefined}
allowFontScaling={false}
Expand Down
4 changes: 4 additions & 0 deletions src/components/TextInput/TextInputLabel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ function TextInputLabel({for: inputId = '', label, labelTranslateY, labelScale,
ellipsizeMode={!isMultiline ? 'tail' : undefined}
ref={textRef(labelRef)}
role={CONST.ROLE.PRESENTATION}
accessible={false}
accessibilityElementsHidden
importantForAccessibility="no"
aria-hidden
style={[styles.textInputLabelContainer, styles.textInputLabel, animatedStyle, styles.pointerEventsNone]}
>
{label}
Expand Down
12 changes: 12 additions & 0 deletions src/components/TextInput/TextInputMeasurement/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,21 @@ function TextInputMeasurement({
onSetTextInputWidth(e.nativeEvent.layout.width);
onSetTextInputHeight(e.nativeEvent.layout.height);
}}
accessible={false}
accessibilityElementsHidden
importantForAccessibility="no"
aria-hidden
>
<Text
style={[
inputStyle,
autoGrowHeight && styles.autoGrowHeightHiddenInput(width ?? 0, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : undefined),
{width: contentWidth},
]}
accessible={false}
accessibilityElementsHidden
importantForAccessibility="no"
aria-hidden
>
{/* \u200B added to solve the issue of not expanding the text input enough when the value ends with '\n' (https://github.com/Expensify/App/issues/21271) */}
{value ? `${value}${value.endsWith('\n') ? '\u200B' : ''}` : placeholder}
Expand All @@ -68,6 +76,10 @@ function TextInputMeasurement({
styles.hiddenElementOutsideOfWindow,
styles.visibilityHidden,
]}
accessible={false}
accessibilityElementsHidden
importantForAccessibility="no"
aria-hidden
onLayout={(e) => {
if (e.nativeEvent.layout.width === 0 && e.nativeEvent.layout.height === 0) {
return;
Expand Down
49 changes: 21 additions & 28 deletions src/pages/signin/SignInPageLayout/Footer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import SignInGradient from '@assets/images/home-fade-gradient--mobile.svg';
import Hoverable from '@components/Hoverable';
import ImageSVG from '@components/ImageSVG';
import Text from '@components/Text';
import type {LinkProps, PressProps} from '@components/TextLink';
import TextLink from '@components/TextLink';
import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset';
import useLocalize from '@hooks/useLocalize';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
Expand All @@ -18,14 +16,11 @@ import Socials from '@pages/signin/Socials';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
import type {SignInPageLayoutProps} from './types';
import FooterRow from './FooterRow';
import type {FooterColumnRow, SignInPageLayoutProps} from './types';

type FooterProps = Pick<SignInPageLayoutProps, 'navigateFocus'>;

type FooterColumnRow = (LinkProps | PressProps) & {
translationPath: TranslationPaths;
};

type FooterColumnData = {
translationPath: TranslationPaths;
rows: FooterColumnRow[];
Expand Down Expand Up @@ -183,27 +178,25 @@ function Footer({navigateFocus}: FooterProps) {
{translate(column.translationPath)}
</Text>
<View style={[styles.footerRow]}>
{column.rows.map(({href, onPress, translationPath}) => (
<Hoverable key={translationPath}>
{(hovered) => (
<View>
{onPress ? (
<TextLink
style={getTextLinkStyle(hovered)}
onPress={onPress}
>
{translate(translationPath)}
</TextLink>
) : (
<TextLink
style={getTextLinkStyle(hovered)}
href={href}
>
{translate(translationPath)}
</TextLink>
)}
</View>
)}
{column.rows.map((row) => (
<Hoverable key={row.translationPath}>
{(hovered) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

The two branches of the ternary render nearly identical FooterRow components -- only the onPress vs href prop differs. Since FooterRow already handles the onPress/href branching internally (both the web and native implementations check if (onPress)), the caller can pass the whole row without splitting:

{column.rows.map((row) => (
    <Hoverable key={row.translationPath}>
        {(hovered) => (
            <FooterRow
                onPress={row.onPress}
                href={row.href}
                translationPath={row.translationPath}
                text={translate(row.translationPath)}
                style={getTextLinkStyle(hovered)}
            />
        )}
    </Hoverable>
))}

This eliminates the duplicated JSX block.


Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

row.onPress ? (
<FooterRow
onPress={row.onPress}
translationPath={row.translationPath}
text={translate(row.translationPath)}
style={getTextLinkStyle(hovered)}
/>
) : (
<FooterRow
href={row.href}
translationPath={row.translationPath}
text={translate(row.translationPath)}
style={getTextLinkStyle(hovered)}
/>
)
}
</Hoverable>
))}
{i === 2 && (
Expand Down
47 changes: 47 additions & 0 deletions src/pages/signin/SignInPageLayout/FooterRow/index.native.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import React from 'react';
import type {GestureResponderEvent, StyleProp, TextStyle} from 'react-native';
import {PressableWithoutFeedback} from '@components/Pressable';
import Text from '@components/Text';
import useEnvironment from '@hooks/useEnvironment';
import useThemeStyles from '@hooks/useThemeStyles';
import type {FooterColumnRow} from '@pages/signin/SignInPageLayout/types';
import {openLink as openLinkUtil} from '@userActions/Link';
import CONST from '@src/CONST';

type FooterRowProps = FooterColumnRow & {
text: string;
style: StyleProp<TextStyle>;
};

function FooterRow({href, onPress, translationPath, text, style}: FooterRowProps) {
const styles = useThemeStyles();
const {environmentURL} = useEnvironment();

return (
<PressableWithoutFeedback
accessible
accessibilityRole={CONST.ROLE.LINK}
accessibilityLabel={text}
sentryLabel={translationPath}
onPress={() => {
if (onPress) {
onPress({} as GestureResponderEvent);
return;
}
if (href) {
openLinkUtil(href, environmentURL);
}
}}
>
<Text
accessible={false}
suppressHighlighting
style={[styles.link, style]}
>
{text}
</Text>
</PressableWithoutFeedback>
);
}

export default FooterRow;
33 changes: 33 additions & 0 deletions src/pages/signin/SignInPageLayout/FooterRow/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import React from 'react';
import type {StyleProp, TextStyle} from 'react-native';
import TextLink from '@components/TextLink';
import type {FooterColumnRow} from '@pages/signin/SignInPageLayout/types';

type FooterRowProps = FooterColumnRow & {
text: string;
style: StyleProp<TextStyle>;
};

function FooterRow({href, onPress, text, style}: FooterRowProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-4 (docs)

The translationPath prop is included in FooterRowProps (via FooterColumnRow) but is never used in the web implementation of FooterRow. The caller in Footer.tsx passes translationPath={row.translationPath} to this component, but it is silently ignored.

Consider either:

  1. Using translationPath for something (e.g., as a data-testid or sentryLabel for consistency with the native version), or
  2. Omitting it from the props type for the web implementation if it is truly not needed here.

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

if (onPress) {
return (
<TextLink
style={style}
onPress={onPress}
>
{text}
</TextLink>
);
}

return (
<TextLink
style={style}
href={href}
>
{text}
</TextLink>
);
}

export default FooterRow;
8 changes: 7 additions & 1 deletion src/pages/signin/SignInPageLayout/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type React from 'react';
import type {ForwardedRef} from 'react';
import type {LinkProps, PressProps} from '@components/TextLink';
import type {TranslationPaths} from '@src/languages/types';

type SignInPageLayoutProps = {
/** The children to show inside the layout */
Expand Down Expand Up @@ -35,4 +37,8 @@ type SignInPageLayoutRef = {
scrollPageToTop: (animated?: boolean) => void;
};

export type {SignInPageLayoutRef, SignInPageLayoutProps};
type FooterColumnRow = (LinkProps | PressProps) & {
translationPath: TranslationPaths;
};

export type {SignInPageLayoutRef, SignInPageLayoutProps, FooterColumnRow};
7 changes: 4 additions & 3 deletions tests/ui/AddressPageTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,13 @@ describe('AddressPageTest', () => {
renderPage(SCREENS.SETTINGS.PROFILE.ADDRESS);

await waitForBatchedUpdatesWithAct();
const state = screen.getAllByLabelText('State / Province');
expect(state.at(1)?.props.value).toEqual('Test');
const stateInput = screen.getByLabelText('State / Province');
expect(stateInput.props.value).toEqual('Test');
Navigation.setParams({
country: 'VN',
});
await waitForBatchedUpdatesWithAct();
expect(state?.at(1)?.props.value).toEqual('Test');
const stateInputAfterParams = screen.getByLabelText('State / Province');
expect(stateInputAfterParams.props.value).toEqual('Test');
});
});
4 changes: 2 additions & 2 deletions tests/ui/IOURequestStepHoursTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ describe('IOURequestStepHours', () => {

await waitForBatchedUpdatesWithAct();

// NumberWithSymbolForm should display the existing hours value
expect(screen.getByText(String(existingHours))).toBeDefined();
// NumberWithSymbolForm should prefill the input with existing hours value
expect(screen.getByDisplayValue(String(existingHours))).toBeDefined();
});
});

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/components/TextInputLabel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('TextInputLabel', () => {
labelScale,
});
// Find the Animated.Text component by its text content
const labelElement = screen.getByText(longLabel);
const labelElement = screen.getByText(longLabel, {includeHiddenElements: true});
// Verify the component renders the correct text
expect(labelElement).toBeTruthy();
// Verify the props for shortening behavior
Expand All @@ -44,7 +44,7 @@ describe('TextInputLabel', () => {
labelScale,
});
// Find the Animated.Text component by its text content
const labelElement = screen.getByText(label);
const labelElement = screen.getByText(label, {includeHiddenElements: true});
// Verify the component renders the correct text
expect(labelElement).toBeTruthy();
// Verify that numberOfLines and ellipsizeMode are undefined
Expand Down
Loading