Page MenuHomePhabricator

[web] Reset tooltip label after a timeout
ClosedPublic

Authored by tomek on Dec 29 2022, 1:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 1:15 AM
Unknown Object (File)
Fri, Nov 22, 4:44 AM
Unknown Object (File)
Fri, Nov 22, 4:44 AM
Unknown Object (File)
Fri, Nov 22, 4:44 AM
Unknown Object (File)
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Dec 29 2022, 1:42 AM
web/utils/tooltip-utils.js
421–438 ↗(On Diff #20297)

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 ↗(On Diff #20297)

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 ↗(On Diff #20297)

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 ↗(On Diff #20297)

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.