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

Fix AutocompleteInput input's value not updating correctly #8238

Merged
merged 10 commits into from
Oct 12, 2022

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented Oct 5, 2022

Fixes #8227

@WiXSL WiXSL added WIP Work In Progress RFR Ready For Review and removed WIP Work In Progress labels Oct 5, 2022
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Besides, it would be nice if you could add some tests to cover this case in the future 🙂

packages/ra-ui-materialui/src/input/AutocompleteInput.tsx Outdated Show resolved Hide resolved
@slax57
Copy link
Contributor

slax57 commented Oct 7, 2022

I'm afraid the fix is still incomplete.

There are still cases where you cannot use backspace, for example in the AutocompleteInput/Basic story:

Peek 07-10-2022 18-47

I think the problem actually comes from this function:

    const handleInputChange = (
        event: any,
        newInputValue: string,
        reason: string
    ) => {
        if (!doesQueryMatchSelection(newInputValue, event?.type)) {
            setFilterValue(newInputValue);
            debouncedSetFilter(newInputValue);
        }
    };

The if makes it so that the input is not updated if the new value is a valid option.
I don't know why this if was added. I tried removing it and everything seems to work just fine without it.

Besides, the unit test should allow to clear the first character is actually wrong.
Actually it should break from what I see in the storybook.
I believe it's because after focus, userEvent.type(input, 'f'); is actually replacing the whole text content instead of appending to it.
We should fix this test, too.

In the end, I'm not sure we need to change the doesQueryMatchSelection callback. Removing the if in handleInputChange seems to be enough.

@WiXSL
Copy link
Contributor Author

WiXSL commented Oct 7, 2022

I'm afraid the fix is still incomplete.

There are still cases where you cannot use backspace, for example in the AutocompleteInput/Basic story:
...

Great! thanks for your review! 👍
I was using the Basic story as well, but testing with the empty choice being selected before typing.
I will follow your findings to finish it

@slax57 slax57 added this to the 4.4.2 milestone Oct 12, 2022
@slax57 slax57 merged commit 2e82307 into master Oct 12, 2022
@slax57 slax57 deleted the fix-autocompleteinput-update-value branch October 12, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutocompleteInput input's value not updating correctly
2 participants