mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.74k stars 32.24k forks source link

[Autocomplete] - Passing props to ListboxComponent #18822

Closed ChrisWiles closed 4 years ago

ChrisWiles commented 4 years ago

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 commented 4 years ago

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

oliviertassinari commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

phancdinh commented 4 years ago

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.

m4theushw commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Would you like me to make a PR for ListboxProps?

Need to update useAutocomplete, docs and add a test?

oliviertassinari commented 4 years ago

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 commented 4 years ago

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

oliviertassinari commented 4 years ago

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 commented 4 years ago

Oh boy https://github.com/mui-org/material-ui/issues/18885 🙃

cmnstmntmn commented 3 years ago

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

ChrisWiles commented 3 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

@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 commented 1 year ago

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