-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add workspace autocomplete list and highlighting #60424
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
Add workspace autocomplete list and highlighting #60424
Conversation
@Expensify/design @flaviadefaria @mountiny @WojtekBoman This pr is pretty much ready. Could you trigger an adhoc build and test it. |
There is a bug currently on main that makes a search router behave weirdly. I fixed it in the last commit so the router can be tested in adhoc builds. |
Will trigger a build now. |
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Highlight is looking pretty good to me! |
4fd0ba8
to
dd58318
Compare
@289Adam289 How is this one looking? |
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 pretty straightforward 👍
I only left minor ideas for code improvements, nothing big.
@@ -159,6 +159,7 @@ function filterOutRangesWithCorrectValue( | |||
case CONST.SEARCH.SYNTAX_FILTER_KEYS.TAX_RATE: | |||
case CONST.SEARCH.SYNTAX_FILTER_KEYS.FEED: | |||
case CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID: | |||
case CONST.SEARCH.SYNTAX_FILTER_KEYS.POLICY_ID: | |||
return substitutionMap[`${range.key}:${range.value}`] !== undefined; |
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.
No bugs here 👍 I noticed a small improvement that we could do.
The way this key is built: ${range.key}:${range.value}
should be the exact same way that we build it inside autocompleteMap.
There is a local getSubstitutionsKey()
function in buildSubstitutionsMap
(and duplicated in 1 other file as well).
We could extract it, and reuse here - just to be super clean 😎
@@ -561,6 +561,63 @@ function getPolicyIDFromSearchQuery(queryJSON: SearchQueryJSON) { | |||
return policyID; | |||
} | |||
|
|||
/** | |||
* A copy of `getFilterDisplayValue` handling the policy ID, used if you have access to the leftHandBar beta. | |||
* When this beta is no longer needed, this method will be renamed to `getFilterDisplayValue` and will replace the old method. |
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.
The cleanest thing we can do here is to mark this with @Todo
and then link the correct issue for the cleanup (I think @WojtekBoman can probably help us find the proper one).
@todo comments stand out more than normal comments
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.
Would be good to tie it to some issue so we do not forget this
I think we are ready for review |
@dukenv0307 @dannymcclain ready for a review |
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
code looks good |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-04-23.at.15.55.10.movAndroid: mWeb ChromeScreen.Recording.2025-04-23.at.15.53.44.moviOS: NativeScreen.Recording.2025-04-23.at.15.54.43.moviOS: mWeb SafariScreen.Recording.2025-04-23.at.15.53.10.movMacOS: Chrome / SafariScreen.Recording.2025-04-23.at.15.47.54.movMacOS: DesktopScreen.Recording.2025-04-23.at.15.58.11.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.
LGTM
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.
Thanks!
@@ -561,6 +561,63 @@ function getPolicyIDFromSearchQuery(queryJSON: SearchQueryJSON) { | |||
return policyID; | |||
} | |||
|
|||
/** | |||
* A copy of `getFilterDisplayValue` handling the policy ID, used if you have access to the leftHandBar beta. | |||
* When this beta is no longer needed, this method will be renamed to `getFilterDisplayValue` and will replace the old method. |
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.
Would be good to tie it to some issue so we do not forget this
✋ 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/mountiny in version: 9.1.32-0 🚀
|
@289Adam289 Any steps for QA team? |
Sorry I forgot about that
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.1.32-8 🚀
|
Explanation of Change
Fixed Issues
$ #59367
PROPOSAL:
Tests
workspace:
Offline tests
QA Steps
workspace:
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Android: Native
Android: mWeb Chrome
Screen.Recording.2025-04-22.at.12.30.10.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2025-04-22.at.12.18.25.mov
MacOS: Chrome / Safari
Screen.Recording.2025-04-22.at.12.05.27.mov
MacOS: Desktop