Page MenuHomePhabricator

[web] introduce the getEmojiKeyboardPosition function
ClosedPublic

Authored by ginsu on Feb 6 2023, 12:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 7:45 AM
Unknown Object (File)
Thu, Mar 21, 7:45 AM
Unknown Object (File)
Fri, Mar 15, 11:39 AM
Unknown Object (File)
Tue, Feb 27, 10:51 PM
Unknown Object (File)
Tue, Feb 27, 10:51 PM
Unknown Object (File)
Tue, Feb 27, 10:51 PM
Unknown Object (File)
Tue, Feb 27, 10:51 PM
Unknown Object (File)
Tue, Feb 27, 10:51 PM
Subscribers

Details

Summary

introduce the getEmojiKeyboardPosition function to reaction-message-utils

getEmojiKeyboardPosition will try to find the best position to place the emoji keyboard. Here is how I ranked the positions of the emoji keyboard:

  1. The best position is to place the emoji keyboard to the right/left of the tooltip where it doesn't cover the message being reacted to. (The first two conditional statements handle this case)
  2. The next best position is to place the emoji keyboard at the top of the tooltip. I ranked the top position over the bottom position purely out of personal preference, but if we want to switch, we can do that. (The third conditional statement handles this case)
  3. The next best position is to place the emoji keyboard at the bottom of the tooltip (The fourth conditional statement handles this case)
  4. If all fails, we can resort to placing the emoji keyboard to the left/right of the tooltip, where it does cover the message being reacted to. This is not ideal, so is ranked last, but is better than having the emoji keyboard overflow off of the screen.

Depends on D6786
Linear Task: ENG-2849

Test Plan

Please watch the demo videos below to see the different cases mentioned above (Please note that D6634 will actually call and implement this logic)

Best case:

2nd best case:

3rd best case:

4th best case:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu added reviewers: atul, tomek, rohan.
ginsu requested review of this revision.Feb 6 2023, 12:36 PM
tomek requested changes to this revision.Feb 15 2023, 2:00 AM
tomek added inline comments.
web/chat/reaction-message-utils.js
132–133

Can we compute these values instead of hardcoding them?

web/utils/tooltip-utils.js
48–55

We should export it as a part of the block at the end of this file.

But overall, extracting this const out of the function is incorrect. When the values were computed in the function, current window dimensions were taken. When the value is extracted, we freeze the values initially and never update, so resizing the window would break the solution.

Please find a solution where we don't freeze the window sizes and include testing resizing of the window as a part of test plan.

This revision now requires changes to proceed.Feb 15 2023, 2:00 AM
web/utils/tooltip-utils.js
48–55

Good catch, I created D6786 to help address this

Looks ok, but I haven't verified the complicated logic. It would be great if we had some tests checking it, or at least it should be covered in a test plan.

web/chat/reaction-message-utils.js
127 ↗(On Diff #22750)

Do we really have to use an invariant? Can we e.g. replace it with returning null?

This revision is now accepted and ready to land.Feb 21 2023, 2:36 AM
web/chat/reaction-message-utils.js
127 ↗(On Diff #22750)

We could replace the invariant with returning null; however, I chose to use an invariant here to add transparency and make it easy to figure out what went wrong if appContainerPositionInfo is not set. If we just return null, the positioning of the emoji keyboard would do the following, and at least to me, is not super clear that this is an error since no red flags were raised.

Let me know what you think, and if we think returning null is a better solution, I'll make the necessary changes to address it.

web/chat/reaction-message-utils.js
127 ↗(On Diff #22750)

I think it's better to have some issues with the picker than crashing the app. This might be null only when window is not present - I guess it might happen for example during server side rendering. So I think it is safer to e.g. not render the picker when this is null - it's a lot better than crashing the app.

This revision was landed with ongoing or failed builds.Mar 2 2023, 12:34 PM
This revision was automatically updated to reflect the committed changes.