Page MenuHomePhabricator

[web] Reset tooltip label after a timeout
ClosedPublic

Authored by tomek on Dec 29 2022, 1:28 AM.
Tags
None
Referenced Files
F3351367: D6087.diff
Sat, Nov 23, 1:15 AM
F3344726: D6087.id20439.diff
Fri, Nov 22, 4:44 AM
F3344725: D6087.id20446.diff
Fri, Nov 22, 4:44 AM
F3344724: D6087.id20297.diff
Fri, Nov 22, 4:44 AM
F3344686: D6087.id.diff
Fri, Nov 22, 4:43 AM
Unknown Object (File)
Tue, Nov 5, 2:08 AM
Unknown Object (File)
Tue, Oct 29, 10:14 AM
Unknown Object (File)
Oct 22 2024, 5:17 PM
Subscribers

Details

Summary

After the previous diff successful action results in updating the label. The problem is that the label will remain updated for the life of the component. In our approach it causes issues because only closing a thread will result in the component being unmounted. So we can't rely on unmounting when resetting the value.

We could try resetting the value on mouse leave, but without significant changes, it isn't possible in this hook to know that the pointer exited the whole tooltip / message. In this hook we can only detect if a pointer exited an icon, but that approach could result in the feedback being shown only for a short period and probably changing unexpectedly to the user.

The solution to these might be a timeout which makes the success message being displayed for some time.

Depends on D6084

Test Plan

Added a couple of console logs to check if the message is updated and if the timeout is cleared after the component unmounts (a thread is switched).

Diff Detail

Repository
rCOMM Comm
Branch
ENG-2332
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Dec 29 2022, 1:42 AM
web/utils/tooltip-utils.js
421–438

This looks like a debounce basically. What do you think of replacing this code with Lodash _debounce? See here for an example

  1. the callback passed to setTimeout is passed to _debounce instead, with copiedMessageDurationMs as the second param. The result is assigned to a variable eg. debounce
  2. onSuccess is replaced with a call to debounce()
  3. clearTimeout is replaced with debounce.cancel()
web/utils/tooltip-utils.js
421–438

I'm not sure what approach do we use in Comm, but code here feels readable and straight forward and introducing library function would force me to look for API and docs to find out what it does. I find sticking to pure JS is good decision here if lodash doesn't introduce significant simplification.

web/utils/tooltip-utils.js
421–438

Initially I thought that replacing this with debounce isn't a good idea. The main reason was that the purpose of debounce is to limit a number of function execution when it is called repeatedly, but here we only need to postpone the execution. I also considered reduced readability caused by using a function that wasn't meant to be used in this context which could be confusing.

But after some time I realized that we're also limiting the number of executions here by clearing the previous timeout and setting a new one. So it seems like debounce can simplify this significantly. I'm going to give it a try.

This revision is now accepted and ready to land.Dec 30 2022, 6:59 AM
web/utils/tooltip-utils.js
421–438

I'm not sure what approach do we use in Comm, but code here feels readable and straight forward and introducing library function would force me to look for API and docs to find out what it does. I find sticking to pure JS is good decision here if lodash doesn't introduce significant simplification.

I think this is a really good instinct. Too often people introduce indirection that makes code more "concise" (shorter) but less readable, and I agree that readability should be a priority.

That said, I think "debounce" is one of those concepts that all frontend programmers eventually become familiar with. It's kind of like RegEx... it's more concise, but more confusing if you don't understand it. But once you understand it, it can be less confusing. And we're already using it once in the codebase

This revision was automatically updated to reflect the committed changes.