-
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
Remove additional letters #18707
Remove additional letters #18707
Conversation
Hey @kaushiktd did this come out of an issue your proposal was accepted for? If so, which one (so I can assign the right team members)? If not, let me know and I'll close this PR - be sure to read our Contributing guidelines before moving forward with PRs - we're excited to have your interest, though! |
Ah cool! Yeah so make sure to use the existing App PR template so that the magic links to the existing issues work! In this case, I'll just assign @johnmlee101 and @mollfpr to this one. Thanks! |
Agreed, please follow the template so our checklists are performed correctly 👍 |
@johnmlee101 @mollfpr I checked guidelines but I could not find the template you have mentioned about. Can you please help here? |
Okay I found it and I think it is ready for review. @johnmlee101 @mollfpr |
@kaushiktd you will still need to add tests on all other platforms to demonstrate that there are no regressions. If you could add appropriate screencapture and checks for those that would be great! |
@johnmlee101 I've added video files for web, Desktop, ios, and android. Note that I have run Expensify on my local system and it generates http://localhost:8080/ URL. So it will work only with the system where we compile the code. Hence, above url is not loading in Mobile Chrome and Safari browser. |
@kaushiktd Do you use your auto format? There are a lot of unnecessary changes happening. Could you revert that and only commit the important changes? |
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
I am getting an error in reverting. Can I create a new PR and close this one? @mollfpr |
@kaushiktd Yup, no problem! |
Details
Remove additional letters after selecting a suggested emoji (in android devices).
Fixed Issues
$ #16106
$ #16106 (comment)
Tests
I've test all the scenario with selected emojis in android and ios platform.
Steps to follow:
Offline tests
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
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)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
Web
Screen.Recording.2023-05-12.at.10.25.29.AM.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
Screen.Recording.2023-05-12.at.10.29.17.AM.mov
iOS
ios.mov
Android
![Screenshot_2023-05-11-10-06-55-56_4f9154176b47c00da84e32064abf1c48](https://github.com/Expensify/App/assets/43398804/4fe7d57c-0c76-40d3-9f83-d664e8890259)Record_2023-05-11-10-07-04.mp4