-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix - Cannot enter address in personal details, address disappears after entering #28474 #30819
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@jjcoffee I've made some changes, I've removed the extra fallback and I've implemented |
Sorry that I keep making changes but what I decided is that I'm already using a regex to capture the street address so there is no point in using ExpensiMark here since it will be passed plain text string anyway and it uses regex in a similar way as well regardless so I don't see the benefit of using it, I think just simple plain regex should suffice here, otherwise we could make a custom parser or use a parsing library, which looking at our package.json it seem like we already have htmlparser2 v7.2.0, although we don't seem to be using it anywhere in the app so not sure why it's there. |
@jjcoffee Thoughts? |
I think I prefer parsing vs regex, and it looks like htmlparser2 is used in a few places. Not a huge deal if it's too much of a pain to implement but it might be worth a try! |
@jjcoffee It's not a pain at all! here's my code to implement htmlparser2 for this, let me know what you think of it, until then I will be doing some testing to see if there seem to be any potential issues with using this code if (!values.street && details.adr_address) {
let streetAddress;
let matchedText = false;
const parser = new HtmlParser({
onopentag(name, attribs){
if(name === "span" && attribs.class === "street-address"){
matchedText = true;
}
},
ontext(text){
if(matchedText){
streetAddress = text;
}
},
onclosetag(tagname){
if(tagname === "span"){
matchedText = false;
}
}
}, { decodeEntities: true });
parser.write(details.adr_address);
parser.end();
if (streetAddress) {
values.street = streetAddress.trim();
}
} |
Ah this doesn't look as tidy as I imagined! I think it might be overkill for this case and we can probably stick with the regex for now, unless you don't want your extra work to go to waste 😄 We can also see what the internal engineer thinks. I'll try to get the checklist done tomorrow, unfortunately higher priority PRs got in the way today! |
@jjcoffee Haha no don't worry about it, I don't mind trying different things to see what feels right, we can certently stick to the regex way, it should work fine either way since we already use similar methods in ExpensiMark anyway :) |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2023-11-09_14.55.43.mp4Android: mWeb Chromeandroid-chrome-2023-11-09_14.51.25.mp4iOS: Nativeios-native-2023-11-09_15.50.46.mp4iOS: mWeb Safariios-safari-2023-11-09_15.40.36.mp4MacOS: Chrome / Safaridesktop-chrome-2023-11-09_14.26.33.mp4MacOS: Desktopdesktop-app-2023-11-09_15.01.48.mp4 |
Co-authored-by: Joel Davies <joeld.dev@gmail.com>
@AmjedNazzal Testing on a distance request, I'm getting a bit of a weird result where only the street number displays: distance-request-2023-11-09_15.54.45.mp4 |
@jjcoffee That's due to how |
Ooh okay yeah I see that looks like it would be out of scope for this issue I think! |
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!
@cristipaval All yours! Just a quick note that we did look at parsing the HTML instead, but it ended up not being as elegant as I had hoped, so I decided it's not worth it for this when a very simple regex check will do. There is also this issue for distance requests, but it seems out of scope as distance requests do some manual parsing of the address for display. |
Agree! Thank you both! |
✋ 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/cristipaval in version: 1.3.99-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.99-0 🚀
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.4.0-0 🚀
|
Details
We are adding a street fallback for instances where neither street_number nor route exist in the API response
Fixed Issues
$ #28474
PROPOSAL: #28474 (comment)
Tests
Offline tests
No test steps as google autocomplete doesn't work offline
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 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)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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-address-details.mp4
Android: mWeb Chrome
mweb-chrome-address-details.mov
iOS: Native
ios-native-address-details.mov
iOS: mWeb Safari
mweb-safari-address-details.mov
MacOS: Chrome / Safari
web-chrome-address-details.mov
MacOS: Desktop
macos-desktop-address-details.mov