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

[Autocomplete] Simplify tooltip usage in options #19254

Closed
2 tasks done
sherodtaylor opened this issue Jan 15, 2020 · 7 comments · Fixed by #22591
Closed
2 tasks done

[Autocomplete] Simplify tooltip usage in options #19254

sherodtaylor opened this issue Jan 15, 2020 · 7 comments · Fixed by #22591
Labels
breaking change 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. new feature New feature or request

Comments

@sherodtaylor
Copy link

sherodtaylor commented Jan 15, 2020

I found an issue with the implementation of autocomplete. If you look at the PR you generate the props from the useAutocomplete hook and pass that directly to the li. This is an issue because #11601 you are passing disabled directly from that which doesn't allow for the Tooltip workaround on the disabled elements here

I've already implemented this in #19235. I feel like this is a valid solution since you are currently able to change the 'ListBoxComponent = ul` to anything. You should have more control over the list item.

Also if you change the ListboxComponent to a div look at the HTML rendering. It renders invalid HTML and you can't change the li to anything you need:

Screen Shot 2020-01-15 at 10 35 00 AM

Heres a demo for the above screenshot:

Edit Material demo

  • 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 😯

The Tooltip isn't opening for disabled items.

Expected Behavior 🤔

I expect Tooltip to render for disabled items. Using the workaround here the disabled items should render.

Steps to Reproduce 🕹

We need to be able to use the Tooltip to explain to our users why an item is disabled. See example below:

Edit Material demo

Steps:

  1. Setup Autocomplete like above ^
  2. Try to render a tooltip
  3. The disabled items will not open the tooltip however the ones, not disabled will.

Context 🔦

We need to be able to use the Tooltip to explain to our users why an item is disabled.

Your Environment 🌎

Check the above code sandbox for more info.

Tech Version
Material-UI v4.8.3
Material-UI-Lab v4.0.0-alpha.39
React 16.12.0
Browser Chrom
TypeScript 3.7.4
@oliviertassinari oliviertassinari added component: autocomplete This is the name of the generic UI component, not the React module! waiting for 👍 Waiting for upvotes labels Jan 15, 2020
@oliviertassinari
Copy link
Member

I have added the waiting for users upvotes tag. I'm closing the issue as we are not sure people are looking for such abstraction. So please upvote this issue if you are. We will prioritize our effort based on the number of upvotes.

@oliviertassinari
Copy link
Member

A note for the core team members, in the future, that will come back to this issue ~10 of the upvotes comes from a single team and can be subtracted from the number of upvotes.

for instance:
  • sherodtaylor - Sherod Taylor - Bloomberg LP.
  • lisahoong - Lisa Hoong - Bloomberg LP.
  • lozzd - Laurie Denness - Bloomberg LP.
  • scottopell - Scott Opell - Bloomberg LP.
  • zohrenweiss - Zohren Weiss - Bloomberg LP.
  • jones77 - James Jones - Bloomberg LP.
  • dskoda1 - David Skoda - Bloomberg LP.

@oliviertassinari oliviertassinari added the new feature New feature or request label Jan 17, 2020
@oliviertassinari oliviertassinari changed the title [Autocomplete] Tooltip inside of autocomplete doesn't highlight when disabled [Autocomplete] Simplify tooltip usage in options Jan 17, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2020

Would it make sense to make this change? Userland:

diff --git a/docs/src/pages/components/autocomplete/CheckboxesTags.tsx b/docs/src/pages/components/autocomplete/CheckboxesTags.tsx
index 97e99d4574..dbc8bcf274 100644
--- a/docs/src/pages/components/autocomplete/CheckboxesTags.tsx
+++ b/docs/src/pages/components/autocomplete/CheckboxesTags.tsx
@@ -18,8 +18,8 @@ export default function CheckboxesTags() {
       options={top100Films}
       disableCloseOnSelect
       getOptionLabel={(option) => option.title}
-      renderOption={(option, { selected }) => (
-        <React.Fragment>
+      renderOption={(props, option, { selected }) => (
+        <li {...props}>
           <Checkbox
             icon={icon}
             checkedIcon={checkedIcon}
@@ -27,7 +27,7 @@ export default function CheckboxesTags() {
             checked={selected}
           />
           {option.title}
-        </React.Fragment>
+        </li>
       )}
       style={{ width: 500 }}
       renderInput={(params) => (

From the implementation side of things, it would mean something close to this to have it work:

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts b/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
index 03a025e822..097cf8fda8 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
@@ -165,11 +165,16 @@ export interface AutocompleteProps<
   /**
    * Render the option, use `getOptionLabel` by default.
    *
+   * @param {object} props The props to apply on the li element.
    * @param {T} option The option to render.
    * @param {object} state The state of the component.
    * @returns {ReactNode}
    */
-  renderOption?: (option: T, state: AutocompleteRenderOptionState) => React.ReactNode;
+  renderOption?: (
+    props: React.HTMLAttributes<HTMLLIElement>,
+    option: T,
+    state: AutocompleteRenderOptionState
+  ) => React.ReactNode;
   /**
    * Render the selected value.
    *
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index 88e1644283..076b4a9c0c 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -375,21 +375,20 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
       <ul className={classes.groupUl}>{params.children}</ul>
     </li>
   );
-
   const renderGroup = renderGroupProp || defaultRenderGroup;
-  const renderOption = renderOptionProp || getOptionLabel;
+
+  const defaultRenderOption = (props2, params) => (
+    <li {...props2}>{getOptionLabel(params.option)}</li>
+  );
+  const renderOption = renderOptionProp || defaultRenderOption;

   const renderListOption = (option, index) => {
     const optionProps = getOptionProps({ option, index });

-    return (
-      <li {...optionProps} className={classes.option}>
-        {renderOption(option, {
-          selected: optionProps['aria-selected'],
-          inputValue,
-        })}
-      </li>
-    );
+    return renderOption({ ...optionProps, className: classes.option }, option, {
+      selected: optionProps['aria-selected'],
+      inputValue,
+    });
   };

   const hasClearIcon = !disableClearable && !disabled;

Does anyone want to work on it? :)

@oliviertassinari oliviertassinari added breaking change ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed waiting for 👍 Waiting for upvotes labels Jun 15, 2020
@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Jul 2, 2020
@ImanMahmoudinasab
Copy link
Contributor

Would it make sense to make this change? Userland:

I checked the changes and I think they make sense.

Does anyone want to work on it? :)

Looks like you already solved the problem and put the code, then what is remained? :D

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 12, 2020

Looks like you already solved the problem and put the code, then what is remained? :D

@ImanMahmoudinasab The hardest part is missing: to get it merged in the next branch. We would need a pull request, fix the tests, update the documentation, etc.

@ImanMahmoudinasab
Copy link
Contributor

@oliviertassinari I'm going to start working on this.

@jonasdev
Copy link

jonasdev commented Jun 25, 2022

I had this issue. Couldn't find any updates?
But I did solve it with this

.MuiAutocomplete-listbox li[aria-disabled="true"] {
  pointer-events: all;
}

.MuiAutocomplete-listbox li[aria-disabled="true"]:hover,
.MuiAutocomplete-listbox li[aria-disabled="true"]:focus {
  background: white;
}

.MuiAutocomplete-listbox li[aria-disabled="true"]:active {
  background: white;
  pointer-events: none;
}

https://codesandbox.io/s/rough-frost-1stq3j

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants