[142] Debounce re-rendering on input changes

lucasdinonolte commented 2 years ago

adds a custom useDebouncedCallback hook
wraps the preview function into the debounced callback, so it'll only get called if there hasn't been an input for 250ms
- exact timeout is very much up for debate, to me 250ms feels like a good compromise between getting performance gains but not feeling unresponsive to the user

Closes #142

netlify[bot] commented 2 years ago

Deploy Preview for dsi-logo-maker ready!

Name	Link
Latest commit	f6017cf0075dc00d80ddbb5d0b1649cf1753812f
Latest deploy log	https://app.netlify.com/sites/dsi-logo-maker/deploys/6363d2400275390009388c0c
Deploy Preview	https://deploy-preview-147--dsi-logo-maker.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

runemadsen commented 2 years ago

Awesome! There's something really odd happening in the inputs for me. This video shows me trying to delete the 1 in 100 and then type a 2 to get to 200. But the 2 ends up in the back of the input field.

https://user-images.githubusercontent.com/192021/188396836-b04d499d-4794-47f0-b562-309f81383d41.mov

lucasdinonolte commented 2 years ago

Awesome! There's something really odd happening in the inputs for me. This video shows me trying to delete the 1 in 100 and then type a 2 to get to 200. But the 2 ends up in the back of the input field.

Screen.Recording.2022-09-05.at.9.48.45.AM.mov

I've had this happen before. I believe this is native input type="number" behavior when a min value is set. When you delete the 1 you make the input drop below its min value, so it automatically "jumps" back to its min value and places the cursor at the back of the input field.

A solution could be to implement our own number input in React using a type="text" input and doing the min, max, number validation ourselves.

lucasdinonolte commented 2 years ago

@fdoflorenzano great ideas. I made it configurable through the settings object

runemadsen commented 2 years ago

Nice! I have two comments up for discussion:

What is the debounce setting is not a boolean but the number of ms to debounce? This would allow users with huge and slow design functions to set a higher debounce and for smaller design functions to set it lower or to nothing at all.
Do we want to consider debouncing at different timeouts per input? I feel like something like a slider needs to quickly generate new versions of the design function in order for the mechanic not to feel laggy. But something like a number input can render slower in order to wait for the user to type the entire number before re-rendering.

fdoflorenzano commented 2 years ago

What is the debounce setting is not a boolean but the number of ms to debounce? This would allow users with huge and slow design functions to set a higher debounce and for smaller design functions to set it lower or to nothing at all.

I was initially thinking this with my previous comment, having a single setting. If it's a number then it's the number of ms to debounce, and it's false then it gets disabled. Currently Lucas added it as separate settings: debounceInputs and debounceDelay.

2. Do we want to consider debouncing at different timeouts per input?

It does make sense, but I have a hard time seeing how to implement it right now. The debouncing Lucas added is applied to the general preview trigger, instead of the individual inputs. So we would need to somehow move the debouncing so that inputs do change their value, but also trigger previews in different debouncing rhythms. A bit more refactoring would probably needed.

runemadsen commented 2 years ago

Thanks for the comments! I'm on board with all of this and I think we should just get this PR in!

lucasdinonolte commented 2 years ago

Thanks for the comments! I'm on board with all of this and I think we should just get this PR in!

My idea of doing two separate settings was to provide the user with a sensible default for debouncing. So not put the "burden" of choosing the correct delay on them unless they really want to.

But maybe we could do something like this:

debounceInputs: false // Default: no debouncing

debounceInputs: true // use default debounce value of 250ms

debounceInputs: 100 // use number as debounce delay

runemadsen commented 2 years ago

I think two separate settings are really nice. I just missed it when I reviewed the design function since it's not in the settings object 👍

lucasdinonolte commented 2 years ago

@runemadsen @bravomartin @fdoflorenzano I'd like to re-open the discussion on debouncing and make an argument in favor of just doing it on all the inputs. I'd even go as far as saying a default debounce of 100ms should be used—giving users the ability to opt-out. Here's why.

Anything below around 200ms feels immediate. So a default debounce of 100ms is below that and the Mechanic UI will still feel very responsive
On the other hand overly simplified assuming the browsers event loop to be running at 60fps we have 16ms between event loops. Without debouncing every event loop tick can potentially start a re-render of the entire function. This is likely to have a negative impact on performance in more complex design functions
- this becomes an issue when the user isn't done inputting
- a new render is started only to be discarded in the next event loop tick again
- also overly simplistic, but with a slight debounce we could save ~6 per debounce interval (16ms * 6 ≅ 100ms)
100ms default debounce feels like a good compromise. To me it feels immediate enough to get a fast response after updating the input values, but the value is still high enough to not re-render like crazy when I just enter a number field or slider and keep pressing on an arrow key
I think the potential performance gains outweigh the hardly perceivable delay between input and re-render

I've updated the deploy preview with the above behavior. Let me know what you think. I'm also very happy to give up on debouncing if you all think it feels too unresponsive.

runemadsen commented 2 years ago

Excellent. I'm convinced.

The only thing related to this that I want to highlight is the ugliness around number inputs. I'm not sure if this is a thing for debounce to save, but I think we need a strategy that makes it possible to change from 100 to 200 by deleting the 1 and typing a 2 without needing to hack it with all sort of weird input combinations.

lucasdinonolte commented 2 years ago

Excellent. I'm convinced.

The only thing related to this that I want to highlight is the ugliness around number inputs. I'm not sure if this is a thing for debounce to save, but I think we need a strategy that makes it possible to change from 100 to 200 by deleting the 1 and typing a 2 without needing to hack it with all sort of weird input combinations.

Interesting idea to use debounce to solve this issue. I think the issue is because the controlled number input clamps the value before emitting to parent. So maybe that component could implement its own debounce, keep track of its display value itself and only emit to the parent after the debounce delay has passed. Is this what you had in mind?

runemadsen commented 2 years ago

Yes, something like that. I think the number input is the only type of input that needs this handling, and I guess it can be introduced in a separate PR, but I wanted to mention it. I vote for getting this PR in.

lucasdinonolte commented 2 years ago

Yes, something like that. I think the number input is the only type of input that needs this handling, and I guess it can be introduced in a separate PR, but I wanted to mention it. I vote for getting this PR in.

Agreed. We could also try if setting the native min and max attributes on the number field helps – thereby making it the browser's problem 😬

Ok, I'd just like to wait to see if @fdoflorenzano agrees with this debouncing and then get this one in :blush:

runemadsen commented 2 years ago

Let's meeeeerge

designsystemsinternational / mechanic