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

[Docs] Autocomplete Multiple Values delete bug #18081

Closed
2 tasks done
Studio384 opened this issue Oct 29, 2019 · 4 comments · Fixed by #18153
Closed
2 tasks done

[Docs] Autocomplete Multiple Values delete bug #18081

Studio384 opened this issue Oct 29, 2019 · 4 comments · Fixed by #18153
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@Studio384
Copy link
Contributor

Studio384 commented Oct 29, 2019

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Clicking the delete button for an option in the "Multiple Values" and "Fixed Options" examples removes whatever is first in the last.

Expected Behavior 🤔

Clicking the delete button for an option removes its own optoin.

Steps to Reproduce 🕹

Link:

  1. https://material-ui.com/components/autocomplete/#multiple-values
  2. https://material-ui.com/components/autocomplete/#fixed-options

Steps:

  1. Add a new option
  2. Try to delete the new option
  3. The option already present will be deleted

Your Environment 🌎

MUI Documentation

@Studio384
Copy link
Contributor Author

On the same page, but unrelated to the issue, the following is written:

The component exposes a factory to create a filter method that can provided to the filerOption prop. You can use it to change the default option filter behavior.

I guess that's supposed to say filterOption and not filerOption.

@oliviertassinari
Copy link
Member

@Studio384 Thank you for the report. I can think of two possible solutions:

  1. We continue with the attribute path, it's relatively simple, outside the IE 11 ponyfill:
diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index bc02f10f1..f43a99e08 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -11,6 +11,26 @@ function stripDiacritics(string) {
     : string;
 }

+// Support IE 11
+// https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
+function closest(element, string) {
+  let closestPonyfill = Element.prototype.closest;
+
+  if (!closestPonyfill) {
+    closestPonyfill = () => {
+      let el = element;
+
+      do {
+        if (el.matches(string)) return el;
+        el = el.parentNode;
+      } while (el !== null && el.nodeType === 1);
+      return null;
+    };
+  }
+
+  return closestPonyfill(string);
+}
+
 export function createFilterOptions(config = {}) {
   const {
     ignoreAccents = true,
@@ -584,7 +604,8 @@ export default function useAutocomplete(props) {
   };

   const handleTagDelete = event => {
-    const index = Number(event.currentTarget.getAttribute('data-tag-index'));
+    const tagRoot = closest(event.currentTarget, '[data-tag-index]');
+    const index = Number(tagRoot.getAttribute('data-tag-index'));
     const newValue = [...value];
     newValue.splice(index, 1);
     handleValue(event, newValue);
  1. We require getTagProps() to collect the tag index. Something like this:
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index 4c6aa5260..eddf2c0cb 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -220,21 +220,20 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
   let startAdornment;

   if (multiple && value.length > 0) {
-    const tagProps = {
-      ...getTagProps(),
+    const getTagProps2 = params => ({
       className: classes.tag,
-    };
+      ...getTagProps(params),
+    });

     if (renderTags) {
-      startAdornment = renderTags(value, tagProps);
+      startAdornment = renderTags(value, getTagProps2);
     } else {
       startAdornment = value.map((option, index) => (
         <Chip
           key={index}
-          data-tag-index={index}
           tabIndex={-1}
           label={getOptionLabel(option)}
-          {...tagProps}
+          {...getTagProps2({ index })}
         />
       ));
     }
diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index bc02f10f1..ea709e7f7 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -583,8 +583,7 @@ export default function useAutocomplete(props) {
     selectNewValue(event, filteredOptions[highlightedIndexRef.current]);
   };

-  const handleTagDelete = event => {
-    const index = Number(event.currentTarget.getAttribute('data-tag-index'));
+  const handleTagDelete = index => event => {
     const newValue = [...value];
     newValue.splice(index, 1);
     handleValue(event, newValue);
@@ -672,8 +671,9 @@ export default function useAutocomplete(props) {
         event.preventDefault();
       },
     }),
-    getTagProps: () => ({
-      onDelete: handleTagDelete,
+    getTagProps: ({ index }) => ({
+      'data-tag-index': index,
+      onDelete: handleTagDelete(index),
     }),
     getPopupProps: () => ({
       role: 'presentation',

I might prefer 2. 🤔

@oliviertassinari oliviertassinari removed their assignment Oct 29, 2019
@joshwooding
Copy link
Member

2 looks better to me as well.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Oct 30, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 30, 2019

@joshwooding Alright, then let's go for it. I like the mental model of it, it makes the getTagProps() similar to the getOptionProps().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants