-
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
[Payment now] [$1000] Compose box - Suggestion list not displays when open group chat and write '@' #24265
Comments
Triggered auto assignment to @conorpendergrast ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
App/src/pages/home/report/ReportActionCompose.js Lines 301 to 312 in 2e9caea
Updated Function Code:
onSelectionChange(e) {
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
const selection = e.nativeEvent ? e.nativeEvent.selection : {};
this.setState({selection});
const shouldWait = this.state.value === '' && getOperatingSystem() === CONST.OS.IOS;
const ms = shouldWait ? 10 : 0;
setTimeout(() => {
if (!this.state.value || selection.end < 1) {
this.resetSuggestions();
this.shouldBlockEmojiCalc = false;
this.shouldBlockMentionCalc = false;
return;
}
this.calculateEmojiSuggestion();
this.calculateMentionSuggestion();
}, ms);
} What alternative solutions did you explore? (Optional)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Compose box - Suggestion list not displays when open group chat and write '@' What is the root cause of that problem?We calculate the mentioned suggestion on onSelectionChange. But onSelectionChange is triggered before onChangeText (native behavior on IOS) What changes do you think we should make in order to solve the problem?We just have the same issue before. @s77rt @mollfpr Ping you here because you have context in #12854 More reference: facebook/react-native#28865 What alternative solutions did you explore? (Optional) |
Reproduced on iOS native app, and not on OSX desktop. |
Job added to Upwork: https://www.upwork.com/jobs/~01597e4b673ae39722 |
Current assignee @conorpendergrast is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
@jeet-dhandha These are confusing. can you update your proposal to clarify the root cause in detail?
How does it create the issue? |
ProposalPlease re-state the problem that we are trying to solve in this issue.On iOS when you launch a chat and type @ the suggestions do not open. What is the root cause of that problem?On iOS the this.state.value updates after the onSelectionChange event only for the first character typed. It seems like the component is not properly mounted and has this side effect that onSelectionChange runs before the value gets set. Resulting in an early return because !this.state.value is true and the calculateMentionSuggestion not running. As soon as you type @ remove it and type @ again it works fine. App/src/pages/home/report/ReportActionCompose.js Lines 301 to 312 in ab99bf4
What changes do you think we should make in order to solve the problem?We should revert this commit changes https://github.com/Expensify/App/pull/19334/files. The issue that these changes aimed to fix are no longer a problem since new upstream changes. Specifically the early return check for !this.state.value needs to be removed in onSelectionChange. It could be moved back to calculateEmojiSuggestion() where it was prior to the changes. And e.nativeEvent.selection.end < 1 does not seem to be making any difference and could also be either moved back or completely removed. What alternative solutions did you explore? (Optional)N/A |
@dukenv0307 Shouldn't the issue fixed by this PR facebook/react-native#35603? |
Thanks for your proposal @WikusKriek Checking it... |
@parasharrajat i will update the proposal with better description. |
Proposal [UPDATED] => (#24265 (comment))Please re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
Updated Function Code:onSelectionChange(e) {
LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));
const selection = e.nativeEvent ? e.nativeEvent.selection : {};
this.setState({selection});
const shouldWait = this.state.value === '' && getOperatingSystem() === CONST.OS.IOS;
const ms = shouldWait ? 10 : 0;
// We need to wait for the `this.state.value` state to be updated before we can calculate the suggestions
// Added setTimeout due to https://github.com/facebook/react-native/issues/28865 (upstream issues)
setTimeout(() => {
// Still keeping this code as to reset the suggestions if the user removes all the text
if (!this.state.value || selection.end < 1) {
this.resetSuggestions();
this.shouldBlockEmojiCalc = false;
this.shouldBlockMentionCalc = false;
return;
}
this.calculateEmojiSuggestion();
this.calculateMentionSuggestion();
}, ms);
} What alternative solutions did you explore? (Optional)
|
@dukenv0307 Does not seem so. @WikusKriek I checked but the issue is still present after I reverted the changes. Can you show a video of the working solution? @jeet-dhandha SetTimeout-based solutions are not preferred and you haven't clarified why Settimout is needed. Please add explanation with solution on how it fits on the system to solve the issue. |
Screen.Recording.2023-08-10.at.15.09.59.mov@parasharrajat this is what I did, main...WikusKriek:App:24265-Suggestion-list-not-display-when-open-group-chat |
Try this with room chat or group chat instead of DM. |
Screen.Recording.2023-08-10.at.16.07.30.mov@parasharrajat is it not working on your side? |
Going to test your diff... |
@WikusKriek's proposal looks good to me. We just need to revert the extra changes. 🎀 👀 🎀 C+ reviewed |
Bump @Beamanator |
📣 @parasharrajat Please request via NewDot manual requests for the Reviewer role ($1000) |
📣 @WikusKriek 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@parasharrajat thanks for the bump, I like @WikusKriek 's proposal as well 👍 |
🎯 ⚡️ Woah @parasharrajat / @WikusKriek, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
This is in production for more than 7 days, but no title update. @Beamanator can you please provide some update here on status and payments. Thanks! |
Yeah, it seems the automation failed on this. |
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
After we fixed the native selection issues in RN, the patch applied in #19334 became redundant and caused this issue.
Regression Test Steps
Do you agree 👍 or 👎 ? |
I missed this today, and will pay this tomorrow 👍 |
Payouts due:Issue Reporter: N/A - Applause
Contributor+: $1000 + 50% urgency = $1500 @parasharrajat - via manual request please! Eligible for 50% #urgency bonus? Yes Upwork job is here. |
Oh wait, hah, this doesn't need a new regression test - it was caught via regression testing. Ok, that's all done now. Closing! |
Payment requested as per #24265 (comment) |
$1,500 payment for @parasharrajat approved based on BZ summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Precondition: user should be signed in and have a group chat with several members
Expected Result:
Suggestion list should be displayed
Actual Result:
Nothing happens
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.51.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Bug6157704_Suggestion_list_not_displayed_on_iphone_14_Pro_Max.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: