-
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
[HOLD for payment 2023-03-02] [$3000] mWeb/safari - Create Group button is hidden by keyboard when selecting participants #11463
Comments
Triggered auto assignment to @alex-mechler ( |
Triggered auto assignment to @laurenreidexpensify ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Triggered auto assignment to @flodnv ( |
Not overdue, reviewing this morning |
This comment was marked as outdated.
This comment was marked as outdated.
In the video, the create group button is hidden when no participant is selected and then shown after one of the participants is selected. Is that expected behavior? Should we always show the "Create Group" button even when there is no selected participant? |
@flodnv @laurenreidexpensify Can you please confirm @wildan-m's comment above? Base on the code it does look like we're hiding the
Second line of the condition in the above snippet. |
I don't know, but I would say yes...?
Why would we do that? Why would we show the |
I think this issue is not valid then. The tester might forgot the expected behavior, or I misinterpreted the issue? |
Oh, great point actually! @kbecciv why would you expect |
@flodnv I'm expecting all environments worked the same, this is only my opinion. Screen_Recording_20221017-104953_New.Expensify.1.mp4 |
Yep, assigning myself. |
|
We had already agreed to a $3,000 payment: #11463 (comment). Title updated for clarity. Given this was a very difficult task and a very difficult regression that nobody could've caught, I'd recommend only a third penalty (or a further increase in the original payout, which is the same result), so let's say the payout will be $2,000. Are you happy with that @rawalyogendra @mananjadhav ? |
It looks like #15543 might have something to do with this as well. |
Awaiting feed back on #11463 (comment) |
Yes I think this would work. Thanks for the consideration @flodnv |
@rawalyogendra could you please check out #15543 ? 😬 |
Seems like we're still waiting here on @rawalyogendra input. |
Hey Everyone, I've been working on this issue for the past few days, but unfortunately, I couldn't find a fix for it. The problem lies in the fact that the VisualViewport Root CauseScreen.Recording.2023-03-08.at.11.41.45.PM.movIf we decide to revert the original PR and subsequent regression fixes, we should still retain some of the changes, such as Line 80 in fb20144
This change will maintain the old behavior on our mWeb(Android) platform, which was recently changed (you can read about it here: https://developer.chrome.com/blog/viewport-resize-behavior/). This new change in Chrome caused similar OSK-related issues on our mWeb(Android) platform too. Lines 52 to 60 in fb20144
This change will fix the issue of mWeb(iOS) scrolling a webpage when there's a text input at the bottom of the page, which was pushing the page title outside the viewport. BeforeScreen.Recording.2023-03-08.at.11.46.39.PM.movAfterScreen.Recording.2023-03-08.at.11.47.55.PM.movIn my opinion, we should open up this issue to other contributors and let them have a say on how we proceed. I'm okay with whatever decision you guys make. Also, just wanted to let you all know that I'm available for any questions or help in fixing this issue. Feel free to reach out. Thank you all for your immense patience. To be honest, diving deep into the rabbit hole of Safari bugs has been a fun adventure for a web developer like me! |
Ok coming back into this issue. Thanks @trjExpensify for carrying this forward while I was away. |
So @flodnv and I were chatting on this one. I want to commend @rawalyogendra for his work in this issue. Further, I agree with @flodnv's comment above regarding payout. I think we kind of have two paths ahead of us, and really in both paths, I think the next steps should be the same. We should close this issue. If we want to open up a new issue and keep this going, that's fine. @flodnv noted that we could re-open #15543. Alternatively, we can also say, this is a Safari thing and not on us, so we just decide to not fix the underlying issue. Whatever the case, I think @rawalyogendra has shown good faith through this journey and we should pay him out for the work done thus far. |
I agree with reopening #15543 to others and try to see if it can be resolved. Current changes have anyway resolved a couple of issues and we should retain them. |
It seems like we all agree so let's move forward like this @JmillsExpensify. Many thanks again to @rawalyogendra for the hard work and to @mananjadhav for the support 🙇 |
Re-opened #15543 so we can continue the convo there. All contributors have been paid out so I'm officially closing this one out! Great work everyone. ❤️ |
...as it was causing app-wise problems.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Issue found when executing PR #11309
Action Performed:
Expected Result:
Create group button is not hidden
Special note: Reiterating for emphasis for the clarity of future contributors! We're only looking for proposals that will address upstream fixes to KeyboardAvoidingView. Thank you!
Actual Result:
Create Group button is hidden when tap on selected user
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.10.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers): any
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Bug5755234_pr_11309.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: