-
Notifications
You must be signed in to change notification settings - Fork 3k
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/28324 : App keeps both right click LHN popup and delete message popup open together #28846
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@thienlnam 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] |
I have read the CLA Document and I hereby sign the CLA |
Reviewer Checklist
Screenshots/VideosWebchrome-desktop-2023-10-13_15.25.48.mp4Mobile Web - Chromeandroid-chrome.mp4Mobile Web - Safariios-safari-2023-10-13_15.43.11.mp4Desktopmac-desktop-2023-10-13_15.36.06.mp4iOSios-native-2023-10-13_15.53.24.mp4Androidandroid-native.mp4 |
@astrohunter62 Please can you add screenshots for all platforms listed, thanks! Also, in the test steps step 4 should start with |
@astrohunter62 Regardless, PRs always need to be tested on all platforms. This is so we don't miss regressions that we might not necessarily expect (this is particularly important in this case as your changes are not limited to web).
Make sure you read carefully through the contributing guide so that you avoid missing out on steps like this. |
Thanks, I am testing and adding screenshots right now. |
@jjcoffee |
@@ -24,6 +25,7 @@ function PopoverContextProvider(props) { | |||
} | |||
activePopoverRef.current.close(); | |||
activePopoverRef.current = null; | |||
ReportActionComposeFocusManager.focus(); |
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.
We can't call this here as it would focus on the composer in all places where this component is used, e.g. when picking a new avatar (AvatarWithImagePicker
).
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.
Does this still work to keep focus on the composer when clicking the same chat?
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.
Yes.
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 is a screenshot for this solution.
1.webm
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 results in the composer not being focused when switching chats. I still think an early return for anything other than a left click in onMouseDown
might be the best bet here.
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.
My solution is working when switching chats. I think onMouseDown
is used to prevent loss of focus when clicking the same chat.
But if you want to use it,
onMouseDown={(e) => {
if (!e) {
return;
}
ReportActionComposeFocusManager.focus();
if (e.button !== 2) e.preventDefault();
}}
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.
Ah my bad, I must have tested incorrectly! Retested and that seems to work. The downside is long-presses focus on the composer still (which would affect tablet users). Do you have any reason you aren't keen on the early return in onMouseDown
?
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.
@jjcoffee I have changed my solution to handle the issue for tablets.
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.
@astrohunter62 Can you push the change so I can test tomorrow? Thanks!
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.
Yup.
@astrohunter62 You have conflicts. I think this may have been fixed in the meantime in #28953 |
I have resolved the conflict. |
@astrohunter62 No, you're welcome to try! |
28953.tablet.webm |
@jjcoffee any updates? |
@astrohunter62 I've asked on the issue to see if we should proceed. Tablet isn't an officially supported platform so I'm a bit unsure. |
@astrohunter62 Can you merge main and I guess you can revert the code from that PR? |
@astrohunter62 Tests well! Once you add comments and revert the change from the other PR, I will approve. |
I have just pushed. |
@astrohunter62 Thanks! We also want to revert this change right?
|
Co-authored-by: Joel Davies <[email protected]>
onMouseDown={(e) => {
if (!e) {
return;
}
`` |
Can I push the code that reverts the above change? |
@astrohunter62 Yes! |
This comment was marked as outdated.
This comment was marked as outdated.
Could you review the PR? |
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!
All yours @MariaHCD! |
@jjcoffee |
Looks like some commits are unverified, @astrohunter62. Can you fix them? As for payment-related questions, those can be on the issue. |
I have resolved. |
✋ 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/MariaHCD in version: 1.3.85-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.3.86-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀
|
Details
App keeps both right click LHN popup and delete message popup open together
Fixed Issues
$ #28324
PROPOSAL: #28324 (comment)
Tests
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
28324.safari.webm
28324.chrome.webm
Mobile Web - Chrome
mobile.chrome.mp4
Mobile Web - Safari
mobile.safari.mp4
Desktop
28324.desktop.webm
iOS
ios.mp4
Android
android.mp4