Details

Reviewers

tomek
kamil
atul
rohan
ginsu

Commits

rCOMM7c6cfdc6a732: [web] Closing modal with outside click changed

Summary

Modals used to be closed with onClick handle, but it caused them to close
with following series of event:

mouseDown event trigerred inside modal
mouse moved cursor moving outside modal
mouseUp event trigerred outside, closing the modal

It could be annoying, as it could close modal in unwanted situations i.e.
user pressing button down to select text in modal, moving cursor
outside, releasing button.
Now it should work as expected, closing modal only when both mouseDown and
mouseUp were triggered in the backgroud of modal.

Test Plan

Built app.
Played around with "Chat settings" modal.
Verified that everything works as exprected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• przemek created this revision.Oct 7 2022, 7:32 AM

Herald added subscribers: • abosh, ashoat. · View Herald TranscriptOct 7 2022, 7:32 AM

• przemek added 1 blocking reviewer(s): tomek.Oct 7 2022, 7:33 AM

Harbormaster completed remote builds in B12672: Diff 17416.Oct 7 2022, 7:43 AM

• przemek requested review of this revision.Oct 7 2022, 7:43 AM

atul added reviewers: rohan, ginsu.Oct 7 2022, 7:47 AM

For this diff, it would be great to see a before and after video so that reviewers can easily/clearly see the changes on a visual level

web/modals/modal.react.js
35 ↗	(On Diff #17416)	nit: use `React.useState` instead of `useState`

This revision now requires changes to proceed.Oct 7 2022, 10:40 AM

tomek requested changes to this revision.Oct 10 2022, 3:29 AM

tomek added inline comments.

web/modals/modal.react.js
35 ↗	(On Diff #17416)	Agree, we should use hooks from `React` explicitly. I'm not sure if using state here is the best idea. When we set state inside the callback, we force a render of the component, without any significant change. I think it would be better to avoid the render and `useRef` instead.

Updating D5315: [web] Closing modal with outside click changed

Changed useState to useRef like tomek suggested.
Adding before and after video
(Let me know if it is okay to include such things in comments or should I rather edit main summary above?)
Before:

After:

• przemek attached a referenced file: F195676: new.mov. (Show Details)Oct 11 2022, 12:57 AM

• przemek attached a referenced file: F195677: old.mov. (Show Details)

Harbormaster completed remote builds in B12719: Diff 17480.Oct 11 2022, 1:02 AM

(Let me know if it is okay to include such things in comments or should I rather edit main summary above?)

It's fine either way (maybe updating the summary is a little better). I might be a good idea to include videos with lower resolution in the future, so that they don't take so much space on the screen.

Additional things we should test:

What happens when we start clicking on the background and finish in the modal
What happens when we start clicking on the background and finish outside the window
What happens when we start clicking on the modal and finish outside the window

It is also a really good idea to be more specific in test plan and specify which scenarios were tested

web/modals/modal.react.js
37–41 ↗	(On Diff #17480)	This could be simplified
48 ↗	(On Diff #17480)	We can simplify this condition

This revision now requires changes to proceed.Oct 11 2022, 3:03 AM

Updating D5315: [web] Closing modal with outside click changed

Fixed code style issues.
Performed additional tests Tomek mentioned.
The app behaves in an expected way in all three cases. It only closes modal when both events happend inside website, but outside modal.

Harbormaster failed remote builds in B12723: Diff 17484!Oct 11 2022, 4:59 AM

Harbormaster completed remote builds in B12723: Diff 17484.Oct 11 2022, 5:25 AM

ginsu requested changes to this revision.Oct 11 2022, 7:01 AM

ginsu added inline comments.

web/modals/modal.react.js
34 ↗	(On Diff #17484)	is `useRef` the best hook for this? It doesn't seem like you are referencing anything in the DOM. Some additional context for why you chose this hook would be super helpful

This revision now requires changes to proceed.Oct 11 2022, 7:01 AM

Hi, @ginsu, I used useRef instead useState, because we don't want to trigger component reload on every click, but we need to hold state with data about last event. If I understand it correctly useRef creates object that can hold any type of data, and one of use cases is to hold temporary states like that. That is also what @tomek suggested in one of previous comments. Let me know if should explicitly state that in comment or is explanation here sufficient?

gotcha that makes sense, thanks for the extra context!

This revision now requires review to proceed.Oct 11 2022, 7:21 AM

Looks good! We only need to check one thing from inline comment.

web/modals/modal.react.js
95–96 ↗	(On Diff #17484)	Wondering how these events are handled on devices with touchscreen (e.g. when we open web app in phone web browser). Could you check that?

This revision now requires changes to proceed.Oct 12 2022, 2:35 AM

ginsu mentioned this in D5353: [lib] introduced components/modal to lib.Oct 12 2022, 12:38 PM

Updating D5315: [web] Closing modal with outside click changed

Tested what @tomek mentioned. On mobile web, mouseup and mousedown are only triggered on single tap in one place. Therefore the app behaves as expected. It closes modal when user taps outside it and doesn't close on any drag events.

Also did a little refactor, so ref actually holds ref (instead of boolean) as @ginsu mentioned