Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug in TextInputs when scrolling #1838

Merged
merged 12 commits into from Feb 26, 2024
Merged

Conversation

Mil4n0r
Copy link
Collaborator

@Mil4n0r Mil4n0r commented Feb 19, 2024

Checklist
(Check off all the items before submitting)

  • Build process is done without errors. All tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
Fixes bug related to #1800 where, on scroll, text inputs where displaying NaN.
The solution has been adding the pre-condition of the field being a number field before applying the handleWheel callback.

@Mil4n0r Mil4n0r marked this pull request as ready for review February 20, 2024 11:32
@GomezIvann GomezIvann self-requested a review February 20, 2024 14:54
@GomezIvann GomezIvann self-assigned this Feb 20, 2024
Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

  • Since you are conditionally adding an event listener, you should also check the same condition when cleaning the effect. If not, you are always removing a non-existing event.
  • handleWheel is missing the dependencies incrementNumber and decrementNumber in the useCallback.
  • Please, format the code before committing it.
  • Should we add a test to avoid this from happening in the future (checking that the wheel increases the input value).

@Mil4n0r Mil4n0r marked this pull request as draft February 21, 2024 07:34
@Mil4n0r Mil4n0r marked this pull request as ready for review February 21, 2024 09:12
Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can solve the useEffect dependencies issue splitting the onWheel functionality:

  1. Use the onWheel event of the Input component to do the increment/decrement functionality (remember to remove the event.preventDefault, is a passive event):
    const handleNumberInputWheel = (event: React.WheelEvent<HTMLInputElement>) => {
      if (document.activeElement === inputRef.current)
        event.deltaY < 0 ? incrementNumber(inputRef.current.value) : decrementNumber(inputRef.current.value);
    };  

onWheel={numberInputContext?.typeNumber === "number" ? handleNumberInputWheel : undefined}

  1. Remove the useEffect from the TextInput and move it to the NumberInput. This makes sense since this functionality should only be available in that component:
    const numberInputRef = React.useRef<HTMLInputElement>(null);

    useEffect(() => {
      const input = numberInputRef.current?.getElementsByTagName("input")[0] as HTMLInputElement;
      const preventDefault = (event: WheelEvent) => {
        event.preventDefault();
      };

      input?.addEventListener("wheel", preventDefault, { passive: false });
      return () => {
        input?.removeEventListener("wheel", preventDefault);
      };
    }, []);

    return (
      <NumberInputContext.Provider value={{ typeNumber: "number", minNumber: min, maxNumber: max, stepNumber: step }}>
        <NumberInputContainer ref={numberInputRef}>

What do you think?

lib/src/text-input/TextInput.tsx Outdated Show resolved Hide resolved
@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Feb 26, 2024

We can solve the useEffect dependencies issue splitting the onWheel functionality:

  1. Use the onWheel event of the Input component to do the increment/decrement functionality (remember to remove the event.preventDefault, is a passive event):
    const handleNumberInputWheel = (event: React.WheelEvent<HTMLInputElement>) => {
      if (document.activeElement === inputRef.current)
        event.deltaY < 0 ? incrementNumber(inputRef.current.value) : decrementNumber(inputRef.current.value);
    };  

onWheel={numberInputContext?.typeNumber === "number" ? handleNumberInputWheel : undefined}

  1. Remove the useEffect from the TextInput and move it to the NumberInput. This makes sense since this functionality should only be available in that component:
    const numberInputRef = React.useRef<HTMLInputElement>(null);

    useEffect(() => {
      const input = numberInputRef.current?.getElementsByTagName("input")[0] as HTMLInputElement;
      const preventDefault = (event: WheelEvent) => {
        event.preventDefault();
      };

      input?.addEventListener("wheel", preventDefault, { passive: false });
      return () => {
        input?.removeEventListener("wheel", preventDefault);
      };
    }, []);

    return (
      <NumberInputContext.Provider value={{ typeNumber: "number", minNumber: min, maxNumber: max, stepNumber: step }}>
        <NumberInputContainer ref={numberInputRef}>

What do you think?

This looks like a much more logical approach, encapsulating the NumberInput functionality encapsulated within the appropiate component and reducing all the unnecessary dependencies inside the TextInput component.

Thanks for your time 😄

@GomezIvann GomezIvann merged commit 51152f3 into master Feb 26, 2024
4 checks passed
@GomezIvann GomezIvann deleted the Mil4n0r/fix-NaN-textinput branch February 26, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants