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 onBlur callback on NumberInput and AutocompleteInput #9730

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yanchesky
Copy link
Contributor

Fixes #9726

@yanchesky
Copy link
Contributor Author

Any clues why mobile.cy.js test faills? 🤔 All e2e tests are passing locally (although once mobile.cy.js failed)

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

I don't know why the mobile e2e tests are failing, you'll have to investigate.

@@ -153,7 +153,9 @@ export type InputProps<ValueType = any> = Omit<
export type UseInputValue = {
id: string;
isRequired: boolean;
field: ControllerRenderProps;
field: Omit<ControllerRenderProps, 'onBlur'> & {
onBlur: (...event: any[]) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use React's built-in FocusEventHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I relied on implementation of onBlur function in useInput

const onBlur = useEvent((...event: any[]) => {
controllerField.onBlur();
if (initialOnBlur) {
initialOnBlur(...event);
}
});

and onBlur of InputProps where type is written in such way.

There are also 2 cases where the FocusEventHandler will fail the type contract.

  1. A component needs to blur the input (BooleanInput, RichTextInput, FileInput, CheckboxGroupInput) because FocusEventHandler requires event as an argument.
const handleChange = useCallback(
        event => {
            field.onChange(event);
            // Ensure field is considered as touched
            field.onBlur();
        },
        [field]
    );
  1. RichTextInput provides EditorEvents['blur'] instead, which doesn't match with the FocusEvent.

Here is the solution I came up that matches type contracts. If you like it i'll commit it

 field: Omit<ControllerRenderProps, 'onBlur'> & {
        onBlur: (
            event?:
                | FocusEvent<HTMLInputElement | HTMLTextAreaElement>
                | EditorEvents['blur']
        ) => void;
    };

@yanchesky
Copy link
Contributor Author

I don't know why the mobile e2e tests are failing, you'll have to investigate.

yanchesky#1
On my fork, all tests pass, leading me to suspect that this failure is not related to committed changes.

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.

NumberInput's onBlur prop does not provide an event object
2 participants