[Search v2] [App] Create search results page by WojtekBoman · Pull Request #45409 · Expensify/App

WojtekBoman · 2024-07-15T15:29:06Z

Details

This PR adjusts the header of the Search page to display the custom query. The header displays the user's query when the route param isCustomQuery is set to true. This PR covers also adding the Filters button to the header.

Fixed Issues

$ #45028
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Navigate to the url with the isCustomQuery route param set to true, for example: https://dev.new.expensify.com:8082/search?q=type%3Aexpense%20status%3Aall&isCustomQuery=true
Verify if the header is displayed correctly

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-07-26.at.16.35.24.mov

Android: mWeb Chrome

Screen.Recording.2024-07-26.at.15.59.01.mov

iOS: Native

Screen.Recording.2024-07-26.at.16.42.35.mov

iOS: mWeb Safari

Screen.Recording.2024-07-26.at.15.59.35.mov

MacOS: Chrome / Safari

Screen.Recording.2024-07-26.at.16.52.31.mov

MacOS: Desktop

Screen.Recording.2024-07-26.at.16.53.37.mov

…s-page-with-new-syntax

Kicu

Good job, code looks pretty nice I left some minor comments

Kicu · 2024-07-26T12:54:10Z

    containerStyles?: StyleProp<ViewStyle>;
+
+    /** Whether the subtitle should be displayed above the title */
+    showSubtitleAboveTitle?: boolean;


Im not sure, but If I recall correctly, then our Header for SearchResults is temporary until v2.3. So if this is a temporary header, then do we have to modify the core component Header?
Could we do some dirty fix instead, and leave a comment that it's temporary?

Im afraid that in future we will change our Search header, but nobody will remember to clean up components/Header.
Does this make sense and is there anything you can come up with?

melvin-bot · 2024-07-26T14:57:53Z

@ikevin127 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]

luacmartins

Looking good. Left a few comments

BUG: Other filters don't show up in the supertitle. For example, manually changing the URL to /search?q=type%3Aexpense%20status%3Aall%20category%3AUncategorized&isCustomQuery=true still shows the canned search, instead of type:expense status:all category:Uncategorized

luacmartins · 2024-07-26T15:15:09Z

 };

-function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false}: HeaderProps) {
+function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, showSubtitleAboveTitle = false}: HeaderProps) {


NAB supertitle is a common word used for this subtitle above title

Suggested change

function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, showSubtitleAboveTitle = false}: HeaderProps) {

function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, shouldShowSupertitle = false}: HeaderProps) {

luacmartins · 2024-07-26T15:15:51Z

    searchType: SearchDataTypes;
    isMobileSelectionModeActive?: boolean;
    setIsMobileSelectionModeActive?: (isMobileSelectionModeActive: boolean) => void;
+    isCustomQueryMode?: boolean;


Suggested change

isCustomQueryMode?: boolean;

isCustomQuery?: boolean;

luacmartins · 2024-07-26T15:18:32Z

+            <Button
+                onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}
+                text={translate('search.filtersHeader')}
+                icon={Expensicons.Filters}
+                medium
+            />


I think we should remove this button until some of the filters work, e.g. type, status, date and categories that are already merged/in review. Then we add this button to start showing this. Otherwise, we'd either have to block merging this PR until those filters work or it'd be really confusing to navigate to the filter page when it doesn't do anything.

Suggested change

<Button

onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}

text={translate('search.filtersHeader')}

icon={Expensicons.Filters}

medium

/>

luacmartins · 2024-07-26T15:19:05Z

    [SCREENS.SEARCH.CENTRAL_PANE]: {
        q: SearchQueryString;
-        isCustomQuery: boolean;
+        isCustomQuery: string;


Why this change? isCustomQuery implies a bool to me

It's a bit misleading because this parameter is stored as a string in route.params, and we need to parse it to boolean when we read it in the component

yeah this looks surprising but I believe its correct to type it like this. Everything that comes from route query params will always be a string I believe

Hmm maybe we should create custom param parser in the linking config? It would simply parse string to boolean earlier to keep boolean in the state. It's weird that we use string for something like that.

luacmartins · 2024-07-26T15:19:46Z

+    const {policyIDs, q} = route.params;

-    const {policyIDs} = route.params;
+    const isCustomQueryMode = SearchUtils.isCustomQuery(route.params);


Let's rename all of these instances

Suggested change

const isCustomQueryMode = SearchUtils.isCustomQuery(route.params);

const isCustomQuery = SearchUtils.isCustomQuery(route.params);

luacmartins · 2024-07-26T15:20:21Z

+            <Button
+                icon={Expensicons.Filters}
+                onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}
+                innerStyles={styles.searchNarrowFilterButton}
+            />


Same here

Suggested change

<Button

icon={Expensicons.Filters}

onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}

innerStyles={styles.searchNarrowFilterButton}

/>

ikevin127 · 2024-07-26T18:36:09Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.webm

Android: mWeb Chrome

android-mweb.webm

iOS: Native

ios.mp4

iOS: mWeb Safari

ios-mweb.mp4

MacOS: Chrome / Safari

MacOS: Desktop

luacmartins · 2024-07-29T19:25:06Z

To me, we should loop through the AST tree and parse the values stored there to the capitalized text.

We should use SearchUtils.getFilters and pick only the filters that are needed, i.e. everything in SYNTAX_FILTER_KEYS and type and status in SYNTAX_ROOT_KEYS

App/src/CONST.ts

Lines 5289 to 5310 in 410a6f2

    
           SYNTAX_ROOT_KEYS: { 
        
               TYPE: 'type', 
        
               STATUS: 'status', 
        
               SORT_BY: 'sortBy', 
        
               SORT_ORDER: 'sortOrder', 
        
           }, 
        
           SYNTAX_FILTER_KEYS: { 
        
               DATE: 'date', 
        
               AMOUNT: 'amount', 
        
               EXPENSE_TYPE: 'expenseType', 
        
               CURRENCY: 'currency', 
        
               MERCHANT: 'merchant', 
        
               DESCRIPTION: 'description', 
        
               FROM: 'from', 
        
               TO: 'to', 
        
               CATEGORY: 'category', 
        
               TAG: 'tag', 
        
               TAX_RATE: 'taxRate', 
        
               CARD_ID: 'cardID', 
        
               REPORT_ID: 'reportID', 
        
               KEYWORD: 'keyword', 
        
           },

ikevin127 · 2024-07-29T19:59:53Z

Also, I'm not sure if it's been reported, but the bottom sheet is missing the scrim/overlay:

@dubielzyk-expensify That's only happening on local dev due to StrictMode, I debunked it in #46096 (comment).

To recap, based on the latest comments on the discrepancies reported in #45409 (comment) this is what I gather:

~~1. Wide Layout - Filter labels are lowercase and missing spaces~~

✅ Design team confirmed in #45409 (comment) that the correct design is the one shown in the design doc. We also got confirmation in #45409 (comment) that the Filters lowercase / no-spaces is to be left untouched for now - no changes required here.

~~2. Narrow Layout - Incorrect filter dropdown icon color and padding between icons and text~~

✅ Design team confirmed in #45409 (comment) that we don't currently use the green icon in this case, therefore we shouldn't change that here - no changes required here.

~~3. Narrow Layout - Filter button icon not centered on native app~~

✅ Looks like the Filter button with icon only was removed in the meantime via commit bcd2f8d, therefore this is not an issue anymore - no other changes required here.

4. Narrow Layout - Filter dropdown opens search selection modal (this one is probably NAB but I wanted to bring this up because I wasn't sure)

✅ This issue was addressed by design team in #45409 (comment) and #45409 (comment) and was demonstrated by author in #45409 (comment) as the correct behaviour.

Please correct me if I left anything out!

♻️ Given the above context, I think I can move forward w/ completing the reviewer checklist.

🟢 Update: Completed PR Reviewer Checklist. Will Approve once all code change requests are addressed and conflicts resolved - will perform a double-check on the functionality.

WojtekBoman · 2024-07-30T15:04:18Z

@ikevin127 All comments have been resolved :) Could you recheck it?

luacmartins

LGTM and tests well. @ikevin127 all yours!

ikevin127

LGTM 🚀

luacmartins · 2024-07-30T18:58:25Z

@WojtekBoman we have conflicts 😞 Otherwise, looks good to be merged

WojtekBoman · 2024-07-30T20:04:05Z

@luacmartins Solved :)

luacmartins · 2024-07-30T20:42:04Z

@WojtekBoman :hero:

OSBotify · 2024-07-30T20:53:59Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-07-31T19:30:49Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.15-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-08-02T02:18:43Z

🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.15-9 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

WojtekBoman added 2 commits July 15, 2024 17:24

Add template of Search Results Page

cc9cfce

Merge branch 'main' into search-v2/search-results

03d2a51

luacmartins self-requested a review July 16, 2024 17:07

WojtekBoman added 4 commits July 22, 2024 10:54

Merge branch 'main' into search-v2/search-results

25e6034

Run prettier

9f9b5ef

Merge branch 'adamgrzybowski/use-new-query-syntax' into search-result…

0c146d6

…s-page-with-new-syntax

Adjust SearchResults to queryJSON

ec25f9c

luacmartins mentioned this pull request Jul 25, 2024

[Search v2] [App] Use new query syntax #45617

Merged

50 tasks

WojtekBoman added 7 commits July 26, 2024 08:36

Merge branch 'main' into search-v2/search-results

90227ac

Remove unnecessary components

9e60aca

Adjust components styles to display search results

a39a5f3

Replace texts with translation paths

f89a196

Unify checks in SearchPageHeader

68c9bfa

Remove console.log

db1600e

Remove Filters button

e1e50c6

Kicu suggested changes Jul 26, 2024

View reviewed changes

WojtekBoman added 6 commits July 26, 2024 15:38

Add Filters buttons on narrow and wide layouts

c6c9fca

cleanup destructuring route.params in SearchPage

2c296e7

Rename isSearchResultsMode to isCustomQueryMode

067f484

Add SearchUtils.isCustomQuery and cleanup SearchPageHeader

05dc344

Fix SearchStatusMenuNarrow

10bd38f

Fix filters.svg

dda75e4

WojtekBoman marked this pull request as ready for review July 26, 2024 14:57

WojtekBoman requested review from a team as code owners July 26, 2024 14:57

melvin-bot Bot removed the request for review from a team July 26, 2024 14:57

melvin-bot Bot requested a review from ikevin127 July 26, 2024 14:57

luacmartins requested changes Jul 26, 2024

View reviewed changes

luacmartins reviewed Jul 29, 2024

View reviewed changes

Comment thread src/libs/SearchUtils.ts Outdated

WojtekBoman added 5 commits July 30, 2024 10:19

Merge branch 'main' into search-v2/search-results

01ed964

Modify getSearchHeaderTitle to use getFilters

4b999a4

Refactor operatorsToSignMap

6141a77

Fix displaying search header title

1074c1b

Remove SearchUtils.isCustomQuery

5919064

Remove unused imports

8b2bd3c

luacmartins approved these changes Jul 30, 2024

View reviewed changes

luacmartins requested a review from Kicu July 30, 2024 16:48

ikevin127 approved these changes Jul 30, 2024

View reviewed changes

melvin-bot Bot requested a review from luacmartins July 30, 2024 17:24

luacmartins previously approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'main' into search-v2/search-results

023ecf4

WojtekBoman dismissed luacmartins’s stale review via 023ecf4 July 30, 2024 19:37

Fix optionality of isCustomQuery in SearchListWithHeader

6cffd47

luacmartins approved these changes Jul 30, 2024

View reviewed changes

luacmartins merged commit 8942513 into Expensify:main Jul 30, 2024

github-actions Bot mentioned this pull request Jul 31, 2024

Deploy Checklist: New Expensify 2024-07-31 #46607

Closed

melvin-bot Bot mentioned this pull request Aug 2, 2024

[HOLD for payment 2024-08-09] [Search v2.1] [App] Create search results page #45028

Closed

1 task

rayane-d mentioned this pull request Aug 8, 2024

Search filters: tax, expense type and tag #46736

Merged

49 tasks

rayane-d mentioned this pull request Aug 15, 2024

[Awaiting checklist] [$250] [Search v2] [POLISH] Icons Overflow from Dropdown Button on Search Results Page on Mobile #46664

Closed

6 tasks

	function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, showSubtitleAboveTitle = false}: HeaderProps) {
	function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, shouldShowSupertitle = false}: HeaderProps) {

	const isCustomQueryMode = SearchUtils.isCustomQuery(route.params);
	const isCustomQuery = SearchUtils.isCustomQuery(route.params);

Uh oh!

Conversation

WojtekBoman commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Kicu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melvin-bot Bot commented Jul 26, 2024

Uh oh!

luacmartins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikevin127 commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

luacmartins commented Jul 29, 2024

Uh oh!

Uh oh!

ikevin127 commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WojtekBoman commented Jul 30, 2024

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

ikevin127 left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Jul 30, 2024

Uh oh!

WojtekBoman commented Jul 30, 2024

Uh oh!

luacmartins commented Jul 30, 2024

Uh oh!

OSBotify commented Jul 30, 2024

Uh oh!

OSBotify commented Jul 31, 2024

Uh oh!

OSBotify commented Aug 2, 2024

Uh oh!

Reviewers

WojtekBoman commented Jul 15, 2024 •

edited

Loading

luacmartins left a comment •

edited

Loading

ikevin127 commented Jul 26, 2024 •

edited

Loading

ikevin127 commented Jul 29, 2024 •

edited

Loading