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

[ui-materialui]: Hide <AutoCompleteInput> "Create" option for empty input #10228

Closed
wants to merge 22 commits into from

Conversation

erwanMarmelab
Copy link
Contributor

@erwanMarmelab erwanMarmelab commented Sep 22, 2024

Problem

The “create option on-the-fly” UI of Autocomplete inputs is confusing. I can create an option with "" as value.
Fixes #10043

Solution

  • Don’t display “Create” on the input unless the user types something
  • Fix the brief flash of Create xxx -> xxx

How To Test

  • run ONLY=ra-ui-materialui make storybook
  • go to http://localhost:9010/?path=/story/ra-ui-materialui-input-autocompleteinput--on-create
  • clear the input and see that you cannot create an empty option

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (if not possible, describe why)
    - [ ] The PR includes one or several stories (if not possible, describe why)
    - [ ] The documentation is up to date

Screencasts

Before flash fix:
Capture vidéo du 2024-09-25 19-42-25.webm
After:
Capture vidéo du 2024-09-25 19-43-16.webm

@erwanMarmelab erwanMarmelab added the RFR Ready For Review label Sep 22, 2024
Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Looks good otherwise 💪

packages/ra-ui-materialui/src/input/AutocompleteInput.tsx Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/input/AutocompleteInput.tsx Outdated Show resolved Hide resolved
@slax57 slax57 self-requested a review October 8, 2024 14:57
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.

Other than that it seems to work great! Well done!

@@ -513,7 +519,7 @@ If you provided a React element for the optionText prop, you must also provide t
// add create option if necessary
const { inputValue } = params;
if (onCreate || create) {
if (inputValue === '') {
if (inputValue === '' && createLabel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the docs should be updated to reflect this change: https://marmelab.com/react-admin/AutocompleteInput.html#createlabel

id: choicesForCreationSupport.length + 1,
name: filter,
};
choicesForCreationSupport.push(newOption);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've had issues while testing stories like this one, or OnCreate, which I think are due to the fact that running choicesForCreationSupport.push(newOption) does not always trigger a rerender.
I believe choicesForCreationSupport should be turned into a React State to avoid such problems.

In the following recording, the first item I create ('new') does not appear immediately, nor does it appear in the list. But when I create a second item ('new2'), then, suddenly, both items appear in the list.

Capture.video.2024-10-08.18.16.39.mp4

Comment on lines -368 to -371
const newAuthorName = window.prompt(
'Enter a new author',
filter
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the prompt from this story (which is a good thing IMO because it was redundant with the ability to fill in the filter), reveals a pre-existing odd behavior: if I click on the "Start typing to create a new item" label, then the value is filled with "Start typing to create a new item", which is probably not what I want. This special entry in the list should not be clickable IMO.
But this can be addressed in another PR since it was already existing.

Capture.video.2024-10-08.18.21.45.mp4

@slax57 slax57 requested a review from fzaninotto October 9, 2024 08:26
@erwanMarmelab erwanMarmelab added the DNM Do Not Merge label Oct 9, 2024
@erwanMarmelab
Copy link
Contributor Author

erwanMarmelab commented Oct 9, 2024

DO NOT MERGE -> revert commits are here
New clean PR -> #10266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do Not Merge RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<Autocomplete onCreate> briefly displays wrong option name
3 participants