-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Changes from 6 commits
dab45f6
af13d3c
0e4795a
14aa485
b24e1db
e3b1bc0
9b07663
40ed580
0d75b83
15c24ff
a6f182c
0309363
b7106ff
f7f90b6
259b17d
a13ff39
daa1b24
7751407
3c120c3
d5d502c
b3e4504
7228052
6fa007d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.movWeb: Screen.Recording.2024-09-23.at.1.26.33.AM.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What browser are you using here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cdOut I'm using Chrome There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we need both: |
||||||||||
|
||||||||||
/** 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); | ||||||||||
} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
}; | ||||||||||
|
||||||||||
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]} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ function BaseTextInput( | |
isMarkdownEnabled = false, | ||
excludedMarkdownStyles = [], | ||
shouldShowClearButton = false, | ||
shouldUseDisabledStyles = true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
prefixContainerStyle = [], | ||
prefixStyle = [], | ||
suffixContainerStyle = [], | ||
|
@@ -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} | ||
|
There was a problem hiding this comment.
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
ormessage
or similar.Content suggests to me that this is a more complex structure, and at first I was confused why we have both
children
andcontent