-
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 native KeyboardAvoidingView component everywhere #11586
Conversation
…board avoiding view
…on the Ipad appears in the wrong position (halfway up the page)
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.
Recordings: #11586 (comment)
Checklist: #11586 (comment)
Based on current Tests, On iPad, a blocking view is added to chat list while the keyboard is active
C+ Reviewed
🎀 👀 🎀
@tgolen |
@ctkochan22 Can you please have a final look and merge this? |
I'm approving a payment of $750 for this 👍 |
Thanks 👍 @tgolen |
Everything is checked off, i'm going to merge |
@ctkochan22 looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
✋ 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 @ctkochan22 in version: 1.2.22-0 🚀
|
@jliexpensify For this PR, @Santhosh-Sellavel was the C+ that reviewed this. I believe they normally get paid $250 for reviewing. Because this was such a large PR and process, I have approved for them to be paid $750 instead. Does that make sense? |
Created job: https://www.upwork.com/ab/applicants/1587571561956552704/job-details Invited @Santhosh-Sellavel. Just to confirm: are we waiting the requisite 7 days after deployment, or am I ok to pay now? |
Hi, hope you are doing well. I read issue #11586 on GitHub and found that the theKeyboardAvoidingView issue I will resolve it. React Native provide a KeyboardAvoidingView I will rap the view inside this that cause a problem and it will automatically adjust its height position and bottom padding depending upon the keyboard size. It will work on all platforms. |
@jliexpensify You can still wait the requisite 7 days. @Daniabbasi25 Thanks for taking an interest on this issue. We have implemented the |
🚀 Deployed to production by @Julesssss in version: 1.2.22-3 🚀
|
Accepted invite, thanks! |
Cool, I'll make a note to pay in 2 days! |
Heads up! There is a regression from this PR #12480 |
Thanks Rajat - I'll hold off on payment then. |
@jliexpensify
|
Thanks for digging that up @Santhosh-Sellavel , I'll pay now. |
Paid @Santhosh-Sellavel and job has been closed in Upworks. |
<AnimatedIcon src={Expensicons.Plus} fill={fill} /> | ||
</AnimatedPressable> | ||
</Tooltip> | ||
<KeyboardAvoidingView behavior="padding"> |
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.
This was a mistake to add because it created duplicate keyboard avoiding views. This was caught in the issue here: #10905
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.
The other one is in BaseScreenWrapper
@@ -1,18 +0,0 @@ | |||
/* | |||
* This is a KeyboardAvoidingView only enabled for ios && disabled for all other platforms |
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.
When this file was removed, it led to a lot of regressions by having Android use <KeyboardAvoidingView>
. We found in #11087 that it's best to not use <KeyboardAvoidingView>
on Android at all.
There are a couple of extra things that I threw into this PR that don't need any testing.
[Tracking Issue]
Fixed Issues
$ #11118
$ #10905
Tests
🎯 LHN - The green + button moves
iPad, Android and iOS
Web / Desktop
Do the same test as above. There won't be any keyboard, so just verify that the page looks correct and there are no weird styles or gaps anywhere.
Screenshot iPad
Screenshot iPad mobile web
Screenshot Android native
Screenshot Android mobile web
Screenshot iOS native
Screenshot iOS mobile web
Screenshot Web
Screenshot Desktop
🎯 Keyboard covers input on a chat with a draft
Android and iOS only
Web / Desktop
Do the same test as above. There won't be any keyboard, so just verify that the page looks correct and there are no weird styles or gaps anywhere.
Screenshot Android native
Screenshot Android mobile web
Screenshot iOS native
Screenshot iOS mobile web
Screenshot Web
Screenshot Desktop
PR Review Checklist
PR Author Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
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)QA Steps
Follow all the steps in the above testing section