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

[Search v2.4] Update Search results page (Search bar only) #49462

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
dab45f6
rework SearchRouterInput and SearchPageHeader to use new query result…
cdOut Sep 19, 2024
af13d3c
fix prettier
cdOut Sep 19, 2024
0e4795a
add search change and submit methods
cdOut Sep 19, 2024
14aa485
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 19, 2024
b24e1db
remove unnecessary code and add shouldUseDisabledStyles attribute for…
cdOut Sep 19, 2024
e3b1bc0
fix prettier and lint errors
cdOut Sep 19, 2024
9b07663
resolved review suggestions
cdOut Sep 20, 2024
40ed580
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 20, 2024
0d75b83
fix design suggested styling
cdOut Sep 25, 2024
15c24ff
move styles directly into input
cdOut Sep 25, 2024
a6f182c
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 26, 2024
0309363
refactor duplicate code after merge
cdOut Sep 26, 2024
b7106ff
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 30, 2024
f7f90b6
resolve ts errors after merge and replace BaseTextInput with TextInput
cdOut Sep 30, 2024
259b17d
fix prettier and cleanup unused imports
cdOut Sep 30, 2024
a13ff39
set router input state to disabled
cdOut Sep 30, 2024
daa1b24
refactor small code changes for review
cdOut Sep 30, 2024
7751407
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 30, 2024
3c120c3
Merge branch 'main' into @cdOut/search-results-bar
cdOut Sep 30, 2024
d5d502c
fix eslint errors
cdOut Sep 30, 2024
b3e4504
Merge branch 'main' into @cdOut/search-results-bar
cdOut Oct 1, 2024
7228052
add right padding to optional component of SearchRouterInput
cdOut Oct 1, 2024
6fa007d
fix prettier
cdOut Oct 1, 2024
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
81 changes: 34 additions & 47 deletions src/components/Search/SearchPageHeader.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, {useMemo} from 'react';
import type {StyleProp, TextStyle} from 'react-native';
import {View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import Button from '@components/Button';
Expand Down Expand Up @@ -33,59 +32,50 @@ import type {SearchDataTypes, SearchReport} from '@src/types/onyx/SearchResults'
import type DeepValueOf from '@src/types/utils/DeepValueOf';
import type IconAsset from '@src/types/utils/IconAsset';
import {useSearchContext} from './SearchContext';
import SearchRouterInput from './SearchRouter/SearchRouterInput';
import type {SearchQueryJSON} from './types';

type HeaderWrapperProps = Pick<HeaderWithBackButtonProps, 'title' | 'subtitle' | 'icon' | 'children'> & {
subtitleStyles?: StyleProp<TextStyle>;
type HeaderWrapperProps = Pick<HeaderWithBackButtonProps, 'icon' | 'children'> & {
content: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this something more simple like text or message or similar.
Content suggests to me that this is a more complex structure, and at first I was confused why we have both children and content

isCannedQuery: boolean;
};

function HeaderWrapper({icon, title, subtitle, children, subtitleStyles = {}}: HeaderWrapperProps) {
function HeaderWrapper({icon, children, content, isCannedQuery}: HeaderWrapperProps) {
const styles = useThemeStyles();

// If the icon is present, the header bar should be taller and use different font.
const isCentralPaneSettings = !!icon;

const middleContent = useMemo(() => {
return (
<Header
title={
<Text
style={[styles.mutedTextLabel, styles.pre]}
numberOfLines={1}
>
{title}
</Text>
}
subtitle={
<Text
numberOfLines={2}
style={[styles.textLarge, subtitleStyles]}
>
{subtitle}
</Text>
}
/>
);
}, [styles.mutedTextLabel, styles.pre, styles.textLarge, subtitle, subtitleStyles, title]);

return (
<View
dataSet={{dragArea: false}}
style={[styles.headerBar, isCentralPaneSettings && styles.headerBarDesktopHeight]}
>
<View style={[styles.dFlex, styles.flexRow, styles.alignItemsCenter, styles.flexGrow1, styles.justifyContentBetween, styles.overflowHidden]}>
{icon && (
<Icon
src={icon}
width={variables.iconHeader}
height={variables.iconHeader}
additionalStyles={[styles.mr2]}
{isCannedQuery ? (
<View style={[styles.dFlex, styles.flexRow, styles.alignItemsCenter, styles.flexGrow1, styles.justifyContentBetween, styles.overflowHidden]}>
{icon && (
<Icon
src={icon}
width={variables.iconHeader}
height={variables.iconHeader}
additionalStyles={[styles.mr2]}
/>
)}
<Header subtitle={<Text style={[styles.textLarge, styles.textHeadlineH2]}>{content}</Text>} />
<View style={[styles.reportOptions, styles.flexRow, styles.pr5, styles.alignItemsCenter, styles.gap4]}>{children}</View>
</View>
) : (
<View style={styles.pr5}>
<SearchRouterInput
disabled
isFullWidth
wrapperStyle={styles.searchRouterInputResultsStyle}
defaultValue={content}
shouldShowRightComponent
rightComponent={children}
/>
)}

{middleContent}
<View style={[styles.reportOptions, styles.flexRow, styles.pr5, styles.alignItemsCenter, styles.gap4]}>{children}</View>
</View>
</View>
)}
</View>
);
}
Expand Down Expand Up @@ -153,11 +143,8 @@ function SearchPageHeader({queryJSON, hash, onSelectDeleteOption, setOfflineModa

const isCannedQuery = SearchUtils.isCannedSearchQuery(queryJSON);

const headerSubtitle = isCannedQuery ? translate(getHeaderContent(type).titleText) : SearchUtils.getSearchHeaderTitle(queryJSON, personalDetails, cardList, reports, taxRates);
const headerTitle = isCannedQuery ? '' : translate('search.filtersHeader');
const headerIcon = isCannedQuery ? getHeaderContent(type).icon : Illustrations.Filters;

const subtitleStyles = isCannedQuery ? styles.textHeadlineH2 : {};
const headerIcon = getHeaderContent(type).icon;
const headerContent = isCannedQuery ? translate(getHeaderContent(type).titleText) : SearchUtils.getSearchHeaderTitle(queryJSON, personalDetails, cardList, reports, taxRates);

const headerButtonsOptions = useMemo(() => {
if (selectedTransactionsKeys.length === 0) {
Expand Down Expand Up @@ -302,10 +289,9 @@ function SearchPageHeader({queryJSON, hash, onSelectDeleteOption, setOfflineModa

return (
<HeaderWrapper
title={headerTitle}
subtitle={headerSubtitle}
icon={headerIcon}
subtitleStyles={subtitleStyles}
content={headerContent}
isCannedQuery={isCannedQuery}
>
{headerButtonsOptions.length > 0 ? (
<ButtonWithDropdownMenu
Expand All @@ -320,6 +306,7 @@ function SearchPageHeader({queryJSON, hash, onSelectDeleteOption, setOfflineModa
/>
) : (
<Button
innerStyles={!isCannedQuery && styles.highlightBG}
text={translate('search.filtersHeader')}
icon={Expensicons.Filters}
onPress={onPress}
Expand Down
62 changes: 46 additions & 16 deletions src/components/Search/SearchRouter/SearchRouterInput.tsx
Copy link
Contributor

@rayane-djouah rayane-djouah Sep 23, 2024

Choose a reason for hiding this comment

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

Bug on web: On the web platform, scrolling through the input box to view the overflowing text is only possible when the cursor is directly over the text. However, on the desktop version, you can correctly scroll when the cursor is over the text or any vertical empty space within the input box.

Desktop:

Screen.Recording.2024-09-23.at.1.35.20.PM.mov

Web:

Screen.Recording.2024-09-23.at.1.26.33.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What browser are you using here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cdOut I'm using Chrome

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI very soon in a separate PR we will make this input editable and not disabled. Can someone verify if this bug would also happen when the input is enabled? because if no then we could ignore this for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

sigh, tested this myself and the same behavior is there if the input is not disabled.
How big of a bug do we consider this? Scrolling on text behaves ok, perhaps that is enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a blocker for this PR. We can definitely address it as a follow up.

Original file line number Diff line number Diff line change
@@ -1,38 +1,68 @@
import React, {useState} from 'react';
import type {ReactNode} from 'react';
import {View} from 'react-native';
import type {StyleProp, ViewStyle} from 'react-native';
import BaseTextInput from '@components/TextInput/BaseTextInput';
import useThemeStyles from '@hooks/useThemeStyles';
import variables from '@styles/variables';
import CONST from '@src/CONST';

type SearchRouterInputProps = {
onChange?: (searchTerm: string) => void;

onSubmit?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comments to these 2 props as well to be consistent


/** Whether the input is full width */
isFullWidth: boolean;
onChange: (searchTerm: string) => void;
onSubmit: () => void;

/** Default value for text input */
defaultValue?: string;

/** Whether the input is disabled */
disabled?: boolean;

/** Any additional styles to apply */
wrapperStyle?: StyleProp<ViewStyle>;

/** Should render component on the right */
shouldShowRightComponent?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we need both: rightComponent and shouldShow prop for it.
If rightComponent is passed and is nonNull then just display it, if it's undefined/not passed then don't show it.


/** Component to be displayed on the right */
rightComponent?: ReactNode;
};

function SearchRouterInput({isFullWidth, onChange, onSubmit}: SearchRouterInputProps) {
function SearchRouterInput({isFullWidth, onChange, onSubmit, defaultValue = '', disabled = false, wrapperStyle, shouldShowRightComponent = false, rightComponent}: SearchRouterInputProps) {
const styles = useThemeStyles();

const [value, setValue] = useState('');
const [value, setValue] = useState(defaultValue);

const onChangeText = (text: string) => {
setValue(text);
onChange(text);
if (onChange) {
onChange(text);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (onChange) {
onChange(text);
}
onChange?.(text);

};

const modalWidth = isFullWidth ? styles.w100 : {width: variables.popoverWidth};
const inputWidth = isFullWidth ? styles.w100 : {width: variables.popoverWidth};

return (
<BaseTextInput
value={value}
onChangeText={onChangeText}
onSubmitEditing={onSubmit}
autoFocus
textInputContainerStyles={[{borderBottomWidth: 0}, modalWidth]}
inputStyle={[styles.searchInputStyle, styles.searchRouterInputStyle, styles.ph2]}
role={CONST.ROLE.PRESENTATION}
autoCapitalize="none"
/>
<View style={[styles.flexRow, styles.alignItemsCenter, wrapperStyle ?? styles.searchRouterInputStyle]}>
<View style={styles.flex1}>
<BaseTextInput
disabled={disabled}
shouldUseDisabledStyles={false}
value={value}
onChangeText={onChangeText}
onSubmitEditing={onSubmit}
autoFocus
textInputContainerStyles={[{borderBottomWidth: 0}, styles.ph3, inputWidth]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug on web & desktop: The default cursor is displayed on the disabled text input; however, a text cursor appears on the 12px horizontal padding. I think the cursor should be consistent—either text or default—across the entire input box.

Screenshot 2024-09-23 at 12 09 36 AM
Screen.Recording.2024-09-23.at.12.09.48.AM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug on web: When the input is enabled, the cursor alternates between the default and text cursor as you move it around.

Screen.Recording.2024-09-23.at.1.33.51.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be all fixed, none of those occured anymore when testing on chrome on the latest commit.

inputStyle={[styles.searchInputStyle]}
role={CONST.ROLE.PRESENTATION}
autoCapitalize="none"
/>
</View>
{shouldShowRightComponent && rightComponent}
</View>
);
}

Expand Down
3 changes: 2 additions & 1 deletion src/components/TextInput/BaseTextInput/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function BaseTextInput(
isMarkdownEnabled = false,
excludedMarkdownStyles = [],
shouldShowClearButton = false,
shouldUseDisabledStyles = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's quite unfortunate that we need to add yet another bool flag to this very big component, but I couldn't come up with anything better, so we keep it.
Would be cool to simplify BaseTextInput in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this isn't perfect but there wasn't any other way to disable the default styling when disabling the input. I also second the simplification of BaseTextInput.

prefixContainerStyle = [],
prefixStyle = [],
suffixContainerStyle = [],
Expand Down Expand Up @@ -390,7 +391,7 @@ function BaseTextInput(
: []),

// Add disabled color theme when field is not editable.
inputProps.disabled && styles.textInputDisabled,
inputProps.disabled && shouldUseDisabledStyles && styles.textInputDisabled,
styles.pointerEventsAuto,
]}
multiline={isMultiline}
Expand Down
3 changes: 3 additions & 0 deletions src/components/TextInput/BaseTextInput/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ type CustomBaseTextInputProps = {
/** Whether the clear button should be displayed */
shouldShowClearButton?: boolean;

/** Whether to apply styles when input is disabled */
shouldUseDisabledStyles?: boolean;

/** Style for the prefix */
prefixStyle?: StyleProp<TextStyle>;

Expand Down
6 changes: 5 additions & 1 deletion src/styles/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3603,7 +3603,11 @@ const styles = (theme: ThemeColors) =>
borderRadius: variables.componentBorderRadiusSmall,
borderWidth: 2,
borderColor: theme.borderFocus,
paddingHorizontal: 8,
},

searchRouterInputResultsStyle: {
backgroundColor: theme.highlightBG,
borderRadius: variables.componentBorderRadius,
},

searchTableHeaderActive: {
Expand Down
Loading