-
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
Use SearchRouter everywhere and drop Chat finder #49984
Use SearchRouter everywhere and drop Chat finder #49984
Conversation
ac5ce3f
to
eaaeb5e
Compare
9b35717
to
977bbb5
Compare
977bbb5
to
635e0da
Compare
4204eca
to
aba32d3
Compare
aba32d3
to
078420e
Compare
@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] |
@luacmartins @rayane-djouah This PR enables it all: takes off the Meanwhile if there are problems or deploy blockers then @SzymczakJ should be able to help with those. I understand that we might notice some more bugs since probably now it will be tested more heavily 😅 |
CC @adhorodyski if you mind taking a look at the file |
Nice! Really excited about this one. |
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: Pressing cmd + k
with the modal open closes the modal and reopens it
- Press
cmd + k
to open the modal - With the modal open, press
cmd + k
again - Note that the modal closes and reopens
Expected behavior: the modal should just close if it is already open
@Expensify/design I kicked off an adhoc build and would love your eyes on this one. |
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! Gonna ask the product team to test as well before we get it merged
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Definitely down to try 52px for the height consistency and see how that feels. And if that feels uncomfortably big, we could go down to 40px to match medium button heights? Maybe? |
I'll take care of the changes to the search router input in this PR since the changes won't conflict with the ones in this PR. Also, this is what the recent searches using height: 52px look like. It seems like a lot of empty space to me. |
Saw that we brought this to Slack, but don't think 52px is bad at all. That being said, I think the other alignment fixes are more important than the height |
@Kicu @SzymczakJ we have conflicts |
Conflicts solved. I have a question, will be merging this PR soon or will we be holding up until autocomplete features are on? |
✋ 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.50-0 🚀
|
Looks like a minor regression came out of this PR. Would be great if anyone can make a quick fix #50977 |
I will take a look at this @MonilBhavsar |
🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.50-8 🚀
|
@@ -173,8 +180,12 @@ function SearchRouterList( | |||
ListItem={SearchRouterItem} | |||
containerStyle={[styles.mh100]} | |||
sectionListStyle={[isSmallScreenWidth ? styles.ph5 : styles.ph2, styles.pb2]} | |||
listItemWrapperStyle={[styles.pr3, styles.pl3]} |
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, this caused the following issue: #52158
We need to apply the padding to the pressable instead of the wrapper View.
More details can be found in this proposal: #52158 (comment)
true, | ||
true, | ||
); | ||
Session.checkIfActionIsAllowed(() => { |
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.
We need to prevent the modal from showing during the onboarding process. Ref: #50981
Details
SearchButton
everywhere, replacing oldChatFinder
page<SearchRouter>
is not a separate Page/Screen but rather just a plain react component that is displayed in a modal and accessible from almost every page in the AppChatFinder
is removed, and two e2e tests are rewritten to test the new SearchRourter componentChanged files ESLint check
because I touchedAuthScreens
which should be migrated in a dedicated PRFixed Issues
$ #49123
PROPOSAL:
Tests
cmd + k
opens routerOffline tests
QA Steps
cmd + k
opens routerPR 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
Android: Native
rec-router-andr.mp4
Android: mWeb Chrome
iOS: Native
rec-router-ios-mweb.mp4
iOS: mWeb Safari
rec-router-ios.mp4
MacOS: Chrome / Safari
rec-router-web-1.mp4
rec-router-web-2.mp4
MacOS: Desktop