-
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
Reapply - Add offline search functionality for addresses #43011
Reapply - Add offline search functionality for addresses #43011
Conversation
…ssues/30123" This reverts commit ad20280.
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@dubielzyk-expensify @eVoloshchak One of you needs to 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] |
@neil-marcellini @eVoloshchak @arosiclair Here is the fixed PR, but someone from QA should test this because I'm not able to test everything! I applied the special logic to hide the submit button in the flow, to make sure that the waypoint list would fit the available screen space, when the keyboard showed. At the time I investigated the google autocomplete component and I didn't find a way to make it work with our form container, but I'll take a look into it again. Meanwhile if you have a different idea, please let me know so I can apply it here 😄 |
From a visual perspective this isn't changing anything so as long as the QA holds up for the test cases, then that's fine 👍 |
@neil-marcellini @eVoloshchak @arosiclair Still waiting on reviews 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.
Please include the following test to ensure that we fixed this bug [HOLD for payment 2024-06-11] [HOLD for payment 2024-06-06] IOU - Save button is not above keyboard when waypoint input is in focus
Precondition:
- Recent destinations list has a few addresses
- Go offline
- Go to FAB > Submit expense > Distance
- Tap Start
- Tap on the input
- Verify that the Save button does not appear above the keyboard
- enter some text and hit done on the keyboard
- Verify that the Save button appears again
- Tap into the input and clear the text
- Tap system back button (or swipe) to dismiss the keyboard while the input is still in focus
- Verify that the save button appears
I'm actually kind of torn on point 6. The user can tap out of the input when they're done, or hit "done" on the Android keyboard, and then the save button will show, but it would be a bit more clear to always render the save button. Also, it would be good to check if iOS shows a "Done" button or instead "Enter" which is confusing. Do we really need to hide the save button for extra space? We can discuss on Slack if that's easier. What do you think @arosiclair?
Also, please include tests for all the regressions that happened last time.
The code looks great, except one tiny TS change.
Yeah I'm also having this question now. Based on this frame from the Android test I would say it's fine to just always show the save button. Especially since you can still easily scroll the results list: |
Reviewer Checklist
Screenshots/VideosAndroid: Native24-07-07-23-48-41.mp4Android: mWeb ChromeScreen.Recording.2024-07-07.at.23.36.47.moviOS: NativeScreen.Recording.2024-07-07.at.23.35.18.moviOS: mWeb SafariScreen.Recording.2024-07-07.at.23.33.19.movMacOS: Chrome / SafariScreen.Recording.2024-07-07.at.23.29.16.movMacOS: DesktopScreen.Recording.2024-07-07.at.23.31.12.mov |
listView: [StyleUtils.getGoogleListViewStyle(displayListViewBorder), styles.overflowAuto, styles.borderLeft, styles.borderRight, !isFocused && {height: 0}], | ||
listView: [ | ||
StyleUtils.getGoogleListViewStyle(displayListViewBorder), | ||
listViewOverflow, |
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.
Given the recent changes, is this style still needed?
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.
@eVoloshchak good point! I just removed this 😄 Thanks!
@eVoloshchak I think I finally managed to find a solution that works! Can you try it? |
Gentle bump @eVoloshchak |
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.
UI is looking good, awesome job!
There's a small offline bug though:
- Go offline
- Click on FAB> Submit Expense > Distance tab
- Select a start location
- While on Distance tab, with start location selected, click on location once again to select a new start point
- Notice that only some predefined places are shown (only those that satisfy the search query for currently selected start point)
Screen.Recording.2024-07-02.at.16.37.58.mov
It should show all of the predefined places, same as when online
Screen.Recording.2024-07-02.at.16.37.22.mov
@eVoloshchak just pushed a fix for it! Thanks 😄 |
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.
@pac-guerreiro, offline search is broken by 785a45f
Screen.Recording.2024-07-03.at.21.14.44.mov
# Conflicts: # src/CONST.ts
This reverts commit 785a45f.
@eVoloshchak now it should work properly! 😄 |
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.
Looks good, thanks for sticking with this one guys!
✋ 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/neil-marcellini in version: 9.0.6-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.6-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
// that the google autocomplete component expects for it's "predefined places" feature. | ||
selector: (waypoints) => | ||
(waypoints ? waypoints.slice(0, 5) : []).map((waypoint) => ({ | ||
(waypoints ? waypoints.slice(0, CONST.RECENT_WAYPOINTS_NUMBER as number) : []).map((waypoint) => ({ |
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.
@neil-marcellini @arosiclair @eVoloshchak, this has caused a regression. Can you pls confirm whats the expected fix for that? I think we can slice the predefinedPlaces
array to only show 5 items at a time. It will be still able to search and find option from all the 20 options, we will just slice the array for the UI.
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.
Not a bug, it's working as desired as explained in that regression issue here
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
@@ -304,10 +322,16 @@ function AddressSearch( | |||
}; | |||
}, []); | |||
|
|||
const filteredPredefinedPlaces = useMemo(() => { |
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.
Coming from #48643. If user is online, the filtered predefined places will briefly appear before the server returns the result. :)
Details
Add offline search functionality for addresses
Fixed Issues
$ #30123
PROPOSAL: #30123 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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.-.Native.mp4
Android: mWeb Chrome
Android.-.Web.mp4
iOS: Native
iOS.-.Native.mp4
iOS: mWeb Safari
iOS.-.Web.mp4
MacOS: Chrome / Safari
MacOS.-.Web.mp4
MacOS: Desktop
MacOS.-.Desktop.mp4