-
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
[Search v2.4] Update Search results page (Search bar only) #49462
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
I'm really excited for this one. It's looking good so far! |
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.
Looks mostly good 👍 I left several small comment, please fix them and it's ready to go
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 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
wrapperStyle?: StyleProp<ViewStyle>; | ||
|
||
/** Should render component on the right */ | ||
shouldShowRightComponent?: boolean; |
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 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.
@@ -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 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
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 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
.
type HeaderWrapperProps = Pick<HeaderWithBackButtonProps, 'title' | 'subtitle' | 'icon' | 'children'> & { | ||
subtitleStyles?: StyleProp<TextStyle>; | ||
type HeaderWrapperProps = Pick<HeaderWithBackButtonProps, 'icon' | 'children'> & { | ||
content: string; |
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
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
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid_native.mp4Android: mWeb Chromeandroid_mweb_chrome.mp4iOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-01.at.14.25.19.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-01.at.14.26.05.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-01.at.1.45.08.PM.movMacOS: DesktopScreen.Recording.2024-10-01.at.1.45.40.PM.mov |
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.
(it is currently disabled as autocomplete and support for typed queries will be part of Search 2.5)
@cdOut autocomplete is a 2.5 feature, but typed queries is part of the scope of v2.4. @Kicu already introduced that functionality here, so we need to enable the input on this page. We can handle enabling the input as a follow up if you'd like to keep it separate
Screen.Recording.2024-09-23.at.9.27.57.AM.mov
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.
Looks good! I've left a few minor comments
onChangeText={onChangeText} | ||
onSubmitEditing={onSubmit} | ||
autoFocus | ||
textInputContainerStyles={[{borderBottomWidth: 0}, styles.ph3, inputWidth]} |
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.
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.
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
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.
Should be all fixed, none of those occured anymore when testing on chrome on the latest commit.
if (onChange) { | ||
onChange(text); | ||
} |
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.
if (onChange) { | |
onChange(text); | |
} | |
onChange?.(text); |
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.
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
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.
What browser are you using here?
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.
@cdOut I'm using Chrome
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.
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.
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.
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?
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 don't think this is a blocker for this PR. We can definitely address it as a follow up.
cc @Expensify/design to review the changes: VideoScreen.Recording.2024-09-23.at.1.54.00.AM.movVideoScreen.Recording.2024-09-23.at.1.54.24.AM.mov |
@luacmartins @cdOut
I think doing this in a follow up PR is the way to go. |
@rayane-djouah What color is being used for the background of the input? In Figma, we have it set to Also, for the text inside the input, if possible, we have it laid out in Figma where the name of the filter uses cc @dubielzyk-expensify for a gut check on all that—but that's what I'm seeing in Figma. Also last little thing: can we make it to where when the query gets "cut off" or truncated by the field, we add a little |
@dannymcclain As demonstrated in this comment's recordings, we have enabled horizontal scrolling on the input field. This feature allows users to view and edit text that exceeds the boundaries of the input box. Therefore, adding an ellipsis ("...") at the end should be unnecessary, right? |
@dannymcclain, Yes, it's using
@dannymcclain, I'm not sure if this is within the scope of this PR since it involves markdown formatting (@cdOut Could you please confirm?). Perhaps for now, we could use our standard text color for all the text input instead of the text-supporting color? @cdOut, Based on the Figma mockups I think we need to:
@Expensify/design could you please confirm the above? Current: Mockup: |
I'm not sure what the variable is called in the code. We call it
Totally fair, let's just leave it all as
I actually don't think we need to mess with this Filters button right now. I think we can leave the current colors/sizes of everything as is and just add in the hover state. I think for hover, all we need to do is update the background color. And also, I mentioned this in Figma, but can we add a tiny bit of space to the right of the filters button so that when it's hovered there's equal space around it on all sides? Looks like maybe right now it's about 8px away from the top and bottom, but it touches the edge of the input on the right.
We should be able to use our standard button hover color. We call it For the focused outline, we can totally do that as a follow up and wait until this field isn't disabled. |
Aligned to everything @dannymcclain is saying
Just clarifying that this doesn't show any sort of scroll bar on any platforms right? I agree that we shouldn't use ellipsis for the input field. |
@rayane-djouah ready for a re-review, also merged with the latest main |
<Button | ||
innerStyles={!isCannedQuery && [styles.searchRouterInputResults, styles.borderNone]} | ||
text={translate('search.filtersHeader')} | ||
textStyles={styles.textSupporting} |
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.
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.
That's correct, thank you for catching it!
disabled={disabled} | ||
shouldUseDisabledStyles={false} | ||
textInputContainerStyles={[{borderBottomWidth: 0}]} | ||
inputStyle={[styles.searchInputStyle, inputWidth, styles.pl3, styles.pr3]} |
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.
NAB
inputStyle={[styles.searchInputStyle, inputWidth, styles.pl3, styles.pr3]} | |
inputStyle={[styles.searchInputStyle, inputWidth, styles.ph3]} |
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 would prefer leaving it this way, since using paddingHorizontal
here gets overwritten by internal styling of TextInput (it sets individual paddingLeft
to 0 by default, which has priority over the more generic horizontal styling), leaving only the right padding intact and nullifying the left one. When specifying the paddings directly they get presented correctly.
autoCapitalize="none" | ||
disabled={disabled} | ||
shouldUseDisabledStyles={false} | ||
textInputContainerStyles={[{borderBottomWidth: 0}]} |
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.
textInputContainerStyles={[{borderBottomWidth: 0}]} | |
textInputContainerStyles={styles.borderNone} |
@rayane-djouah I've added the changes and also specified why I'd leave one of the parts of styling as is, let me know if anything else needs fixing before approve |
Resolved merge conflicts with |
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.
LGTM and tests well
@luacmartins all yours! |
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.
LGTM
Big agree! I pointed this out somewhere else. But ideally we'd have equal padding on all sides. Definitely NAB though. |
LGTM! @Expensify/design Here is the current result for your reference: Screen.Recording.2024-10-01.at.3.15.28.PM.movScreen.Recording.2024-10-01.at.3.16.01.PM.mov |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.43-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.43-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.43-6 🚀
|
Details
Reuses SearchRouterInput inside Search results page to display the result search query with a filters button inside itself (it is currently disabled as autocomplete and support for typed queries will be part of Search 2.5).
Fixed Issues
$ #49121
PROPOSAL:
Tests
View results
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
MacOS: Chrome / Safari
CHROME.mov