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] - Passing props to ListboxComponent #18822

Closed
ChrisWiles opened this issue Dec 13, 2019 · 21 comments · Fixed by #18887
Closed

[Autocomplete] - Passing props to ListboxComponent #18822

ChrisWiles opened this issue Dec 13, 2019 · 21 comments · Fixed by #18887
Labels
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

@ChrisWiles
Copy link
Contributor

ChrisWiles commented Dec 13, 2019

Not sure if this is a bug or not.

I am trying to fetch more options when ListboxComponent scroll is at the bottom. Having issues with the custom ListboxComponent.

When I pass props I get a fowardRef warning and the list items won't render correctly even tho fowardRef was used.

      ListboxComponent={listboxProps => (
        <ListboxComponent
          {...listboxProps}
          setIsScrollBottom={setIsScrollBottom}
        />
      )}
const ListboxComponent = forwardRef(function ListboxComponent({ setIsScrollBottom, ...rest }, ref) {
  return (
    <ul
      ref={ref}
      {...rest}
      onScroll={({ target }) =>
        setIsScrollBottom(target.scrollHeight - target.scrollTop === target.clientHeight)
      }
    />
  );
});

Demo
https://codesandbox.io/s/jolly-matsumoto-ugs5d

@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Dec 13, 2019
@oliviertassinari
Copy link
Member

We have a related issue in #18450, I would love to learn more about your load more use case.

@oliviertassinari
Copy link
Member

The ref warning in your codesandbox is expected, it creates a new component at each render without forwarding the ref. You could use the context to be able to listen to your scroll bottom event.

@ChrisWiles
Copy link
Contributor Author

In my case, I am not using the internal filterOptions function.

We are using AWS cloud search. I query the API on throttled keystroke.

A user can search their company’s address book.

When a user searches for “BBQ” AWS may return the first 10 of 45 results.

If the user doesn’t see their match they can either refine the search “BBQ Austin” and return new results or scroll to the bottom of the ListboxComponent and load the next 10 results appending them to the options.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 13, 2019

What prevents you from getting, say, 100 results, instead of 10?

In which case, the pagination is likely no longer useful. I don't think that somebody will scan more than 100 items, the user will likely refine his query after this order of magnitude.

Do you display some sort of indicator at the bottom of the option list to let the user know more options are available/loading?

What do you think of a new ListboxProps prop?

If we had a non-MIT API adapter in the enterprise version of Material-UI for AWS cloud search, would that something you would be potentially interested in?

@ChrisWiles
Copy link
Contributor Author

What prevents you from getting, say, 100 results, instead of 10?

Nothing, but the infinite scroll does look cool

In which case, the pagination is likely no longer useful. I don't think that somebody will scan more than 100 items, the user will likely refine his query after this order of magnitude.

I agree.

Do you display some sort of indicator at the bottom of the option list to let the user know more options are available/loading?

Not at the moment but I am playing around this idea. API results come back in fast enough that not sure it matters for my case but it could for others.

What do you think of a new ListboxProps prop?

I think that would be great. I think it would be the best solution.

If we had a non-MIT API adapter in the enterprise version of Material-UI for AWS cloud search, would that something you would be potentially interested in?

Probably not, not familiar. Didn't know there was an enterprise version

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 13, 2019

What do you think of a new ListboxProps prop?

I think that would be great. I think it would be the best solution.

We have added this xProps prop a couple of times in the past to solve this very problem. I think that it would make a good addition.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 13, 2019
@phancdinh
Copy link

Hi oliviertassinari,
I had same problem, I have 3000 options, So I dont like load all options. When user scroll to bottom, I will load more options.
I think we should have new ListboxProps like onScrollToBottom.
Thank you.

@oliviertassinari oliviertassinari added the new feature New feature or request label Dec 14, 2019
@m4theushw
Copy link
Member

What do you think about this diff?

I still have to adjust the scroll position after options are changed. In this codesandbox the list jumps to the first option when more items are added.

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 5b4ef231c..cec1a01f5 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -88,11 +88,13 @@ export default function useAutocomplete(props) {
     id: idProp,
     includeInputInList = false,
     inputValue: inputValueProp,
+    ListboxProps = {},
     multiple = false,
     onChange,
     onClose,
     onOpen,
     onInputChange,
+    onScrollToBottom,
     open: openProp,
     options = [],
     value: valueProp,
@@ -688,6 +690,12 @@ export default function useAutocomplete(props) {
     setHighlightedIndex(highlightedIndexRef.current);
   });
 
+  const handleListboxScroll = ({ target }) => {
+    if (target.scrollHeight - target.scrollTop === target.clientHeight && onScrollToBottom) {
+      onScrollToBottom();
+    }
+  }
+
   const handlePopupIndicator = event => {
     inputRef.current.focus();
 
@@ -800,6 +808,8 @@ export default function useAutocomplete(props) {
         // Prevent blur
         event.preventDefault();
       },
+      onScroll: handleListboxScroll,
+      ...ListboxProps
     }),
     getOptionProps: ({ index, option }) => {
       const selected = (multiple ? value : [value]).some(

@oliviertassinari
Copy link
Member

In this codesandbox the list jumps to the first option when more items are added.

@m4theushw This codesandbox is not valid, it remounts the Listbox component at each update, I don't think that we can make any decision based on it.

What do you think about this diff?

I don't think that a strict equality check is enough. There is a 8 pixels bottom padding. I would include it to work with keyboard navigation.

Regarding the onScrollToBottom prop, I'm in favor of discouraging the "load more option as we scroll" pattern, so to not support this prop.

@m4theushw
Copy link
Member

m4theushw commented Dec 15, 2019

Regarding the onScrollToBottom prop, I'm in favor of discouraging the "load more option as we scroll" pattern, so to not support this prop.

@oliviertassinari I agree with you, this is a particular case. Also, adding more items would increase the number of DOM elements and decrease performance. An ideal implementation should not create new elements but reuse the ones not visible.

I think that in favor of infinite scroll the developer should add a way to let the user know that more results are available and they can only be visualised in another page. Like what GitHub does.

Screen Shot 2019-12-15 at 12 25 34

But to reproduce the behavior above we don't need the ListboxProps. Just adding another option and custom rendering it would be enough.

@ChrisWiles
Copy link
Contributor Author

Would you like me to make a PR for ListboxProps?

Need to update useAutocomplete, docs and add a test?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 16, 2019

This would be great :) (I don't think that we should add tests, they would slow us down without providing much value for this one)

@ChrisWiles
Copy link
Contributor Author

ChrisWiles commented Dec 16, 2019

What about adding prop passing support to all these too? Just to keep things consistent or is that overkill

  • getInputProps
  • getInputLabelProps
  • getPopupIndicatorProps
  • getClearProps
  • getTagProps
  • getOptionProps

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 16, 2019

Please don't, the fewer we have, the better. I had this problem when doing POCs for styled-components built-in. I suspect we will have to find a systematic solution for v5 anyway.

@oliviertassinari
Copy link
Member

Oh boy #18885 🙃

@cmnstmntmn
Copy link

@ChrisWiles have you find a way to prevent the "jump to start" scrolling glitch?

@ChrisWiles
Copy link
Contributor Author

ChrisWiles commented Feb 1, 2021

@cmnstmntmn

Never heard of that, this is how I set it up

const [isScrollBottom, setIsScrollBottom] = useState(false);
const nearBottom = (target = {}) => {
  const diff = Math.round(target.scrollHeight - target.scrollTop);
  return diff - 25 <= target.clientHeight;
};
  // Autocomplete
      ListboxProps={{
        onScroll: ({ target }) => setIsScrollBottom(nearBottom(target)),
      }}

@RustamY
Copy link

RustamY commented Nov 22, 2021

@ChrisWiles have you find a way to prevent the "jump to start" scrolling glitch?
Hi, maybe:

ListboxProps={{
   onScroll: (event: React.SyntheticEvent) => {
      event.stopPropagation();
      const listboxNode = event.currentTarget;
      const savedScrollTop = listboxNode.scrollTop;
      const diff = Math.round(
         listboxNode.scrollHeight - savedScrollTop,
      );
      if (diff - 25 <= listboxNode.clientHeight) {
         const returnScrollTop = () => {
            setTimeout(() => {
               listboxNode.scrollTop = savedScrollTop;
            }, 0);
         };
         getData(returnScrollTop);
      }
   },
}}

@WasiakSzymon
Copy link

WasiakSzymon commented Dec 14, 2021

After X long hours spent on trying do 'hacks' with scroll/fix warning type listBoxComponent props i couldn't find any working way do handle that... the way which i've chosen is to use hook useAutocomplete and provide custom list box build based on material-ui components like <Typography component='li' /> (i wanted have the same style/look like material-ui autocomplete)

there is a good example (easy to convert to TypeScript) https://codesandbox.io/s/material-demo-0fbyb?file=/demo.js

in my case i only imported getRootProps( for parent div), getInputProps( for inputProps TextField) and everything works

@sadeghhosseiny
Copy link

@oliviertassinari
@WasiakSzymon
@WasiakSzymon

hi,
it is strange but i pass a simple ref to the ListBoxProps as a second argument to the Autocomplete and this bug fix but it raise an error in type script project.
can anyone help me to fix this issue???
this is the error :
Type '{ onScroll: (e: UIEvent<HTMLUListElement, UIEvent>) => void; ref: MutableRefObject; }' is not assignable to type 'HTMLAttributes'.
Object literal may only specify known properties, and 'ref' does not exist in type 'HTMLAttributes'.ts(2322)
Autocomplete.d.ts(163, 3): The expected type comes from property 'ListboxProps' which is declared here on type 'IntrinsicAttributes & AutocompleteProps<any, false, false, false, "div">'

@Ben-CA
Copy link

Ben-CA commented Sep 26, 2023

This thread helped me figure out what I wanted to do all these years later. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants