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

Component Breaks With Long, Variable Height Values #31

Closed
amber-cd opened this issue May 4, 2020 · 17 comments
Closed

Component Breaks With Long, Variable Height Values #31

amber-cd opened this issue May 4, 2020 · 17 comments

Comments

@amber-cd
Copy link

amber-cd commented May 4, 2020

I get the sense this may be a semi-known issue, but I'm filing this as a ticket just in case.

Basically: if you have a list of options which are of variable length, some of which are long enough to wrap onto a new line and some of which are not, the longer options overflow their boundaries and overlap the ones below them. This only happens when there are a large number of options, as a smaller amount of options does not cause this issue and automatically displays the heights appropriately.

I can sort of get around this by using a larger fixed height, but that means that there is a lot of whitespace for any shorter options. I don't know much about the underlying library here... is there potentially a way to keep the dynamic, automatic height calculation of the original implementation instead? I know that the height is used to calculate the size of the window, but if a fixed window height were provided instead, would 'on the fly' sizing for the displayed options then be doable? Or is this just a limitation of the windowing used to make longer lists performant?

Replication here: https://codesandbox.io/s/react-windowed-select-95ybc

(Also, as a side note, I've found that if I use a height format other than a pure number, such as "100px", it breaks so hard I have to force kill my Chrome tab. Easy enough to fix, but it might be worth noting in the documentation that heights must be provided in only that format.)

@jacobworrel
Copy link
Owner

Thanks for opening up an issue @amber-cd .

Dynamic option heights aren't supported with the current implementation because react-window, the library being used for windowing, requires a fixed height prop to determine the height of the window and doesn't support just in time measured content (or "on the fly" sizing). Looks like there's an open issue to support this though: bvaughn/react-window#6

In your example, it's working for shorter lists, because the default windowThreshold of 100 isn't being hit so it actually isn't rendering a windowed menu, but just the default react-select menu.

I opened a separate issue re: the number type for height. This would be a good first issue if you're interested in contributing. I'd be happy to review a PR. Cheers!

@jchamb
Copy link
Contributor

jchamb commented May 27, 2020

@jacobworrel While not a perfect solution you can add Item measuring around the options to force it to the correct height. Pushed a PR off of your 2.0.3 beta branch

@jacobworrel
Copy link
Owner

thanks @jchamb - looks good. will try to carve out some time to review the PR more thoroughly soon.

@amber-cd
Copy link
Author

@jchamb, thanks so much! @jacobworrel do you know if there is any update on this?

@jacobworrel
Copy link
Owner

@amber-cd I reviewed @jchamb 's PR and made a request for some changes.

@jacobworrel
Copy link
Owner

fixed in #38

@vishnu-nt
Copy link

vishnu-nt commented Aug 24, 2020

Hi,
Thanks for the awsome work
Is this fixed? I'm still facing the issue in version 2.0.3.

Replication here: https://codesandbox.io/s/react-windowed-select-forked-7zi33?file=/src/index.js

Is there any workaround?

@amber-cd
Copy link
Author

We had some trouble getting this to work, but we'd thought it was due to a namespace pollution issue with our local NPM repository. Directly cloning the master branch and publishing it to our local repository worked, however.

@vishnu-nt
Copy link

We had some trouble getting this to work, but we'd thought it was due to a namespace pollution issue with our local NPM repository. Directly cloning the master branch and publishing it to our local repository worked, however.

It worked for me as well. Thanks for the quick reply.

@LAKnoKAL
Copy link

@vishnu-nt @amber-cd guys, can you please describe a bit more about your solution? I can't get it to work with the latest version 2.0.3 exactly as @vishnu-nt described previously.

@amber-cd
Copy link
Author

We set up a private NPM repository for ourselves: https://blog.bitsrc.io/how-to-set-up-a-private-npm-registry-locally-1065e6790796

Then we cloned this project from git and published it to the local registry (I bumped it up to a 3.x version to be safe) and it seems to have worked.

I'm starting to suspect at this point that the code in Git is fine but something went wrong when it was published to NPM.

@LAKnoKAL
Copy link

@amber-cd thanks a lot for the explanation, just did the same, and it worked!

@jacobworrel as I understand, once you merge this PR #38 and then push release 2.0.3 it fails with CI build issue and wasn't actually delivered to us.
You resolved this CI build issue by merging the latest bot PR but still, it doesn't help and even your codesandbox example fails with that issue.

The reason I think is that you forgot to publish it to the npm registry.

Hope it helps, and sorry if I'm wrong.

@jacobworrel
Copy link
Owner

thanks @LAKnoKAL - ill look into this when i can

@jacobworrel jacobworrel reopened this Oct 28, 2020
@jacobworrel
Copy link
Owner

@amber-cd @LAKnoKAL I'm able to get this to work in codesandbox with the currently published version (2.0.3): https://codesandbox.io/s/laughing-mclaren-hzxqd?file=/src/App.js

What am I missing?

@LAKnoKAL
Copy link

LAKnoKAL commented Nov 2, 2020

@jacobworrel you missing amount of items. The number of items should be more than the windowThreshold number. So if it's 100 by default then try with 150 items in the list. You start to see it immediately.

Screenshot 2020-11-02 at 08 51 47

@jacobworrel
Copy link
Owner

thanks @LAKnoKAL for pointing that out - I will try republishing and hope that solves this issue.

@jacobworrel
Copy link
Owner

@LAKnoKAL @amber-cd I just republished to npm and it's working as expected now: https://codesandbox.io/s/laughing-mclaren-hzxqd?file=/src/App.js

Thanks again for your help debugging this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants