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

Display an error when duplicate values entered into Autocompelte #2945

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

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Nov 29, 2023

Fixes #2923

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.

@JerryWu1234 JerryWu1234 changed the title mui#2923 Autocomplete will occur an error when set same value fix: #2923 Autocomplete will occur an error when set same value Nov 29, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 21, 2023
@JerryWu1234
Copy link
Contributor Author

image
image

@JerryWu1234 JerryWu1234 marked this pull request as ready for review December 22, 2023 10:42
@zannager zannager added bug 🐛 Something doesn't work support: question Community support but can be turned into an improvement labels Dec 26, 2023
@bharatkashyap bharatkashyap changed the title fix: #2923 Autocomplete will occur an error when set same value Display an error when duplicate values entered into Autocompelte Dec 27, 2023
@JerryWu1234
Copy link
Contributor Author

@apedroferreira help me review this ?

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Good job validating on the 2 different inputs! I left a few suggestions but this is a good start.

There are also a few more cases where it seems that you need to clear the error message or it does not reset, can you please cover all of these? Some examples, not sure if there's any more cases than these:

  • User closes the modal, by either pressing "Close" or clicking outside
  • User clicks "Delete all" to delete all options
  • User presses the back button from an option

@JerryWu1234
Copy link
Contributor Author

@apedroferreira help to review the rest

Copy link
Member

@bharatkashyap bharatkashyap left a comment

Choose a reason for hiding this comment

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

Some notes around the structure of the changes

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 9, 2024
@JerryWu1234
Copy link
Contributor Author

@apedroferreira @bharatkashyap help me to review this PR

Copy link
Member

@bharatkashyap bharatkashyap left a comment

Choose a reason for hiding this comment

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

 switchErrorState((item) =>
        typeof item !== 'string' ? item.value === newOptionValue : item === value,
      );

There is some replication of the above code.

The idea with this feature is that you accept an input value, compare it with the array of existing options, and flip the state of the error in case a duplicate is found. You can refactor this into a single, memoized function that does all of this, with inputText being the only required parameter.

In my opinion, separate validateOptionValue and switchErrorState functions are not needed.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 31, 2024
@JerryWu1234
Copy link
Contributor Author

 switchErrorState((item) =>
        typeof item !== 'string' ? item.value === newOptionValue : item === value,
      );

There is some replication of the above code.

The idea with this feature is that you accept an input value, compare it with the array of existing options, and flip the state of the error in case a duplicate is found. You can refactor this into a single, memoized function that does all of this, with inputText being the only required parameter.

In my opinion, separate validateOptionValue and switchErrorState functions are not needed.

all done

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work PR: out-of-date The pull request has merge conflicts and can't be merged support: question Community support but can be turned into an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete will occur an error when set same value
4 participants