-
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
format request display amount on submit #30873
Conversation
@mananjadhav 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] |
@mananjadhav opened a new PR to trigger automation, hope that's okay. This PR replaces #30652. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-trim-leading-zero.movAndroid: mWeb Chromemweb-chrome-trim-leading-zero.moviOS: Nativeios-trim-leading-zero.moviOS: mWeb Safarimweb-safari-trim-leading-zero.movMacOS: Chrome / Safariweb-request-trim-leading-zero-1.movweb-request-trim-leading-zero-2.movMacOS: Desktopdesktop-trim-leading-zero.movApologies for the delay here. I was out of office. I noticed some lag on running the screens, but it turns out I can see them on main too and that too with my HT account. Hence, nothing to do with this change. |
@jasperhuangg I had this up within 24 hours of being hired, still awaiting final review. If there’s any chance my original timing can be honored (was submitted in time to be merged within 72 hours), I’d appreciate it. |
@erquhart no worries, we usually look at the time of the last commit to decide when the PR was finished |
@jasperhuangg let me know if I should force push a rebase |
quick bump @jasperhuangg on this one. Also can we retrigger the perf-tests. It failed. |
@jasperhuangg can you help push this through? |
Normally rerunning the perf tests should fix things, I'm not really seeing how the changes proposed in this PR could affect them 🤔 |
@jasperhuangg I’m going to push a rebase when I get back to my machine |
@jasperhuangg I force pushed, can we rerun tests |
@jasperhuangg all passed ✅ |
✋ 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 production by https://github.com/luacmartins in version: 1.4.1-13 🚀
|
Details
There is existing formatting that occurs when the request form loads. This PR reuses that formatting on submit, so if the user navigates back after submitting the amount, the amount is consistently formatted to a valid amount.
Fixed Issues
$ #30505
PROPOSAL: #30505 (comment)
Tests
Offline tests
Not connectivity related.
QA Steps
Test steps above apply across all platforms.
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
fix-30505-android.mov
Android: mWeb Chrome
fix-30505-android-mweb.mov
iOS: Native
fix-30505-ios.mov
iOS: mWeb Safari
fix-30505-ios-mweb.mov
MacOS: Chrome / Safari
fix-30505-macos-safari.mov
MacOS: Desktop
fix-30505-desktop.mov