Cancel debounce function after FormField unmounts #6226

jcfilben · 2022-07-08T16:00:10Z

What does this PR do?

Prevents the debounce function from executing if the FormField is unmounted.

Where should the reviewer start?

What testing has been done on this PR?

Tested with the following story and console log statements in the debounce function

import React, { useState } from 'react';
import { Box, Form, FormField, TextInput, Button } from 'grommet';

export const TEMP = () => {
  const [showForm, setShowForm] = useState(true);

  return (
    <Box fill align="center" justify="center">
      <Box width="medium">
        {showForm && <MyForm setShowForm={setShowForm} />}
        <Button
          label="Show form"
          onClick={() => {
            setShowForm(true);
          }}
        />
      </Box>
    </Box>
  );
};

const MyForm = ({ setShowForm }) => (
  <Form
    validate="change"
    onChange={(val) => {
      console.log(`onChange ${val}`);
      setShowForm(false);
    }}
  >
    <FormField
      name="myField"
      validate={(val) => {
        console.log(`validate ${val}`);
      }}
    >
      <TextInput name="myField" placeholder="type here" />
    </FormField>
  </Form>
);

TEMP.storyName = 'TEMP';
TEMP.args = {
  full: true,
};

export default {
  title: 'Input/Form/TEMP',
};

How should this be manually tested?

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Closes #6130
Closes #6129

Screenshots (if appropriate)

Do the grommet docs need to be updated?

It might be good to add the debounceDelay theme prop to the FormField doc. I will create a PR for this.

Should this PR be mentioned in the release notes?

Maybe

Is this change backwards compatible or is it a breaking change?

Backwards compatible

halocline

Tested. Cancelation works as expected. Nice job.

halocline · 2022-07-08T18:41:20Z

src/js/components/FormField/FormField.js

+                      // long depending on how fast people type, and 200ms
+                      // would be an eye blink
+                    },
+                    500,


Wondering if this delay should be something the caller can configure? Especially since we don't know what kind of validation function might be called. There is a theme prop, but seems to have been abandoned...

Yes, I agree that this should be configurable. Since debounceDelay is not being used currently I think we could change it from '300' to '500' and then use it here

ericsoderberghp · 2022-07-11T15:43:07Z

src/js/components/FormField/FormField.js

+                  const debouncedFn = debounce(
+                    () => {
+                      contextOnChange(event);
+                      // A half second (500ms) debounce can be a helpful


Can we move this comment into base.js, since that is where the value is being set?

ericsoderberghp · 2022-07-11T15:45:14Z

src/js/components/FormField/FormField.js

-                    // past it. 2 second (2000ms) might be too long depending
-                    // on how fast people type, and 200ms would be an eye blink
-                  }, 500);
+                  const debouncedFn = debounce(


Can we remove the Fn suffix? It doesn't seem consistent with grommet's naming convention to include typing in variable names.

ericsoderberghp · 2022-07-11T15:49:14Z

src/js/components/FormField/FormField.js

@@ -134,12 +134,14 @@ const Input = ({ component, disabled, invalid, name, onChange, ...rest }) => {
  );
 };

-const debounce = (func, wait) => {
+const debounce = (func, wait, fieldRef) => {


Instead of making debounce() know about the ref, what about changing it to be a hook that will clear the timer automatically when the component is unmounted?

Good idea, I updated to use a hook in the latest commit

A suggestion to tighten it up a bit:

const useDebounce = () => { const [func, setFunc] = useState(); useEffect(() => { const timer = setTimeout(() => func(), theme.global.debounceDelay); return () => clearTimeout(timer); }, [func]); return setFunc; }; ... const debounce = useDebounce(); ... if (contextOnChange) debounce(() => contextOnChange(event)));

untested, but perhaps you get the idea?

Added this in latest commit

ericsoderberghp · 2022-07-16T02:26:35Z

src/js/components/FormField/FormField.js

-                  }, 500);
-                  debouncedFn();
-                }
+                if (contextOnChange) setDebounced(() => contextOnChange);


We seem to have lost the event argument to contextOnChange()

Fixed in latest commit

ericsoderberghp

looking good, one simple comment

ericsoderberghp · 2022-07-18T15:50:47Z

src/js/components/FormField/FormField.js

@@ -194,6 +183,19 @@ const FormField = forwardRef(
    const { formField: formFieldTheme } = theme;
    const { border: themeBorder } = formFieldTheme;

+    const useDebounce = () => {


I think this hook definition can be outside the component, above line 138.

jcfilben added 2 commits July 8, 2022 09:50

Cancel debounce after formfield unmounts

377dc5c

remove console.log

7ced7c2

jcfilben requested a review from halocline July 8, 2022 17:12

halocline approved these changes Jul 8, 2022

View reviewed changes

use debounceDelay from theme

159a5ef

jcfilben requested review from halocline and ericsoderberghp July 11, 2022 15:40

ericsoderberghp reviewed Jul 11, 2022

View reviewed changes

change debounce to be inside of a useEffect to handle cleanup on unmount

ee92511

jcfilben requested a review from ericsoderberghp July 15, 2022 22:17

ericsoderberghp reviewed Jul 16, 2022

View reviewed changes

feedback from review

13526b1

jcfilben requested a review from ericsoderberghp July 18, 2022 15:48

ericsoderberghp reviewed Jul 18, 2022

View reviewed changes

move hook outside component

99e84fc

ericsoderberghp approved these changes Jul 18, 2022

View reviewed changes

Merge branch 'master' into fix/form-debounce

847523b

ericsoderberghp merged commit a6a561e into grommet:master Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cancel debounce function after FormField unmounts #6226

Cancel debounce function after FormField unmounts #6226

Uh oh!

jcfilben commented Jul 8, 2022 •

edited

Loading

Uh oh!

halocline left a comment

Uh oh!

halocline Jul 8, 2022

Uh oh!

jcfilben Jul 8, 2022

Uh oh!

ericsoderberghp Jul 11, 2022

Uh oh!

jcfilben Jul 15, 2022

Uh oh!

ericsoderberghp Jul 11, 2022

Uh oh!

jcfilben Jul 15, 2022

Uh oh!

ericsoderberghp Jul 11, 2022

Uh oh!

jcfilben Jul 15, 2022

Uh oh!

ericsoderberghp Jul 16, 2022

Uh oh!

jcfilben Jul 18, 2022

Uh oh!

ericsoderberghp Jul 16, 2022

Uh oh!

jcfilben Jul 18, 2022

Uh oh!

ericsoderberghp left a comment

Uh oh!

ericsoderberghp Jul 18, 2022

Uh oh!

Uh oh!

Cancel debounce function after FormField unmounts #6226

Cancel debounce function after FormField unmounts #6226

Uh oh!

Conversation

jcfilben commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

halocline left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsoderberghp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcfilben commented Jul 8, 2022 •

edited

Loading