Closed kbecciv closed 10 months ago
Job added to Upwork: https://www.upwork.com/jobs/~01ebdf425a95b3b890
Triggered auto assignment to @sonialiap (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External
)
Suggestions popup doesn't close when dragging a file
We do nothing to close suggestions popup when dragging a file
updateShouldShowSuggestionMenuToFalse
function to receive shouldShow
argument
Update
https://github.com/Expensify/App/blob/35aa3db0fe2a139c6b68cb6d3f6a2b0fd252349e/src/pages/home/report/ReportActionCompose/SuggestionEmoji.js#L112-L119
https://github.com/Expensify/App/blob/35aa3db0fe2a139c6b68cb6d3f6a2b0fd252349e/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L240-L247to
const updateShouldShowSuggestionMenuToFalse = useCallback((shouldShow) => {
setSuggestionValues((prevState) => {
if (prevState.shouldShowSuggestionMenu !== shouldShow) {
return {...prevState, shouldShowSuggestionMenu: shouldShow};
}
return prevState;
});
}, []);
const updateShouldShowSuggestionMenuToFalse = useCallback((shouldShow = false) => {
suggestionEmojiRef.current.updateShouldShowSuggestionMenuToFalse(shouldShow);
suggestionMentionRef.current.updateShouldShowSuggestionMenuToFalse(shouldShow);
}, []);
Add the following code to Suggestions
const {isDraggingOver} = useContext(DragAndDropContext);
...
useEffect(() => {
updateShouldShowSuggestionMenuToFalse(!isDraggingOver);
}, [isDraggingOver])
This works as expected
Mention and emoji suggestions appear over the drop screen when dragging a file on to the composer with suggestions open.
I think the expected behaviour should be not necessarily to close the suggestions popup. Instead we want the 'Drop to Upload' screen to be over the suggestions popup.
We are using this host here to show suggestions.
https://github.com/Expensify/App/blob/71d11e8ee5ef352536a1897988e008f31993857e/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L349
Suggestions are wrapped inside Portal
with this host name here
https://github.com/Expensify/App/blob/71d11e8ee5ef352536a1897988e008f31993857e/src/components/AutoCompleteSuggestions/index.native.js#L8-L11
but here we are porting it to the body
https://github.com/Expensify/App/blob/71d11e8ee5ef352536a1897988e008f31993857e/src/components/AutoCompleteSuggestions/index.js#L56
so the suggestions popover appears over the "Drop to Upload" screen.
I think we should remove createPortal
in the return
of index.js
of AutoCompleteSuggestions
specified above and wrap the View
with Portal
like this
<Portal hostName="suggestions"><View style={StyleUtils.getBaseAutoCompleteSuggestionContainerStyle({left, width, bottom})}>{componentToRender}</View></Portal>
We might wrap the BaseAutoCompleteSuggestions
with Portal
and remove createPortal
in the index.js
of AutoCompleteSuggestions
specified above.
With this change, the componentToRender
and return
of index.js
of AutoCompleteSuggestions
will be something like this.
const componentToRender = (
<Portal hostName="suggestions">
<BaseAutoCompleteSuggestions
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
ref={containerRef}
/>
</Portal>
);
return (
Boolean(width) &&
<View style={StyleUtils.getBaseAutoCompleteSuggestionContainerStyle({left, width, bottom})}>{componentToRender}</View>
);
@s-alves10 Would you publish your branch?
@cubuspl42
Is the suggestions popup closing the expected behaviour?
I think it need not close. In my opinion, the expected behaviour should be that the drop screen should be over the suggestions popup.
@cubuspl42
Please check this branch https://github.com/s-alves10/App/tree/fix/issue-30153
Mention(@) and emoji(:) popup does not close when dragging a file
We don't have the logic to close the suggestion popup when we drag a file
In Suggestions.js
, we can get isDraggingOver
from DragAndDropContext
and use usePrevious
to store the previous state of isDraggingOver
. And then add a useEffect
to close the suggestion popup when isDraggingOver
is changed to true
useEffect(() => {
if (prevIsDraggingOver === isDraggingOver) {
return;
}
if (isDraggingOver) {
updateShouldShowSuggestionMenuToFalse();
}
}, [isDraggingOver, prevIsDraggingOver])
After we send the attachment or close the preview modal, the composer will be focused again and the popup will be opened, so we don't need to update shouldShow
state of suggestion popup again.
this has same root cause(we dont hide popover when dragging over) as https://github.com/Expensify/App/issues/26553 which was decided to close.
@ishpaul777
I think this is apparently a bug and needs to be fixed
cc @cubuspl42
@ishpaul777 Thanks for referencing the earlier issue, as it's always good to know that this was considered before.
I do agree with @s-alves10 that this is an obvious visual glitch.
Mention(@) and emoji(:) popup does not close when dragging a file
We are not closing the Popover when a file is dragging, case not handled
To resolve the issue, Hiding only suggestion and emojipopover when dragover would solve the issue but it would be not enough as there are other popoover which can cause the same issue here IMO we should also apply a global/consistent solution, need to close any popover when a file is dragging over. Add this to PopoverProvider
React.useEffect(() => {
// hide popover on drag enter
const listener = () => {
closePopover();
};
document.addEventListener('dragenter', listener);
return () => {
document.removeEventListener('dragenter', listener);
};
}, [closePopover]);
Note: This will hide any popover menu visibile on Drag enter
https://github.com/Expensify/App/assets/104348397/03510aba-268e-4319-b346-2a1c873ccccf
@c3024
We need to use portal for the issue https://github.com/Expensify/App/issues/27036
@dukenv0307
Mention suggestions wouldn't show when cancelling the dragging in your solution
@ishpaul777
I think your solution would work for other popovers, but we need to show suggestions again after dragging is cancelled or completed
cc @cubuspl42
I am not suggesting to remove the portal altogether.
I think we should use the host "suggestions" instead of document body for the portal.
@cubuspl42 I skimmed the issue and looks like we don't quite have a proposal we want to move forward with, is that correct? Are there any that are close and could be accepted with some changes?
@sonialiap
Sorry for the lack of updates. I think we'll be able to pick a proposal from the existing ones, possibly with small adjustments.
@ishpaul777
It's a good observation that the context menu is also affected! Would it be possible to use our DragAndDropContext
in your solution, instead of subscribing to the DOM events directly?
@cubuspl42
Will you let me know your thoughts on https://github.com/Expensify/App/issues/30153#issuecomment-1774931125?
Mention suggestions wouldn't show when cancelling the dragging in your solution
I think your solution would work for other popovers, but we need to show suggestions again after dragging is cancelled or completed
I don't feel like it's a necessity to automatically show the suggestion popup after the drag is finished. I would personally prefer it if it didn't open automatically; still, both behaviors sound acceptable.
@cubuspl42
I'm saying the case where dragging is cancelled. The composer would get focused again but the suggestion popup doesn't show. We have been treating this as a bug up to now. A similar issue is https://github.com/Expensify/App/issues/22447.
There is something wrong with updateShouldShowSuggestionMenuToFalse
, when opening suggestion menu we set the shouldShowEmojiSuggestionMenu
to true while in updateShouldShowSuggestionMenuToFalse we check for shouldShowSuggestionMenu
this was introduced in https://github.com/Expensify/App/pull/25758
cc @cubuspl42
Would it be possible to use our DragAndDropContext in your solution, instead of subscribing to the DOM events directly?
I beleive we can't use exisiting DragAndDropContext inside the because PopoverProvider is almost at top level heirarcy in isn't wrapped with any DragAndDropProvider
we could wrap it inside a Provider but (opinion) I feel like its just more lines for same result. wdyt?
@cubuspl42
I'm saying the case where dragging is cancelled. The composer would get focused again but the suggestion popup doesn't show. We have been treating this as a bug up to now. A similar issue is #22447.
@s-alves10 Thanks for mentioning this. I'll double-check on the internal Slack channel when I have a chance. Currently there are some technical issues with Slack.
@ishpaul777 I'll come back to the DragAndDropContext
topic once I verify this ^
@cubuspl42, @sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick!
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@cubuspl42 any updates on the things you were going to look into? :)
We've gotten agreement from Puneet for the
a drag gesture overrides all popovers and closes them (until something triggers them again).
plan. @cubuspl42 let's move forward with the proposal that does this?
In my opinion, the best proposal is the one by @ishpaul777, which suggests closing all popovers (whether it's the emoji picker, mention list, or context menu) when a file is dragged over.
In the case of the mentions list, it would be re-opened the next time it's triggered. I find this behavior reasonable and convincing. It was also approved in the Slack thread.
The technical details such as using/not using DragAndDropProvider
can be handled during PR review.
C+ reviewed π π π
Triggered auto assignment to @joelbettner, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@cubuspl42 @joelbettner @sonialiap this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
π£ @cubuspl42 π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @ishpaul777 π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
π£ @daveSeife π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
@cubuspl42 The proposal from @ishpaul777 looks good. I assigned him to this issue.
A draft PR will be up by tomorrow. (thursday)
@sonialiap I reported this bug before here https://github.com/Expensify/App/issues/27564. Could you help to update reporter on the OP
This issue has not been updated in over 15 days. @cubuspl42, @joelbettner, @sonialiap, @ishpaul777 eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
PR is merged
Automation didn't run. The PR went to production on Dec 6, updating issue title to payment on Dec 13 and making payments
@ishpaul777 contributor $500 paid βοΈ @cubuspl42 reviewer $500 paid βοΈ @DylanDylann bug report $50 - paid βοΈ
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.3.88.1 Reproducible in staging?: y Reproducible in production?: y 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 Expensify/Expensify Issue URL: Issue reported by: @daveSeife Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697872753258699
Action Performed:
Expected Result:
Popup closes when dragging a file
Actual Result:
Popup does not close when dragging a file
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/93399543/c696f641-a448-4c1b-b11d-cf67921d1b9d https://github.com/Expensify/App/assets/93399543/f03008a9-6a7c-4464-8060-1a6f0cd75c5bMacOS: Desktop
https://github.com/Expensify/App/assets/93399543/a4ca642b-230c-452f-bd0f-bc477a7d0638View all open jobs on GitHub
Upwork Automation - Do Not Edit