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

[v2]How do I add a menubuffer by version2 ? #2965

Closed
wszgxa opened this issue Aug 23, 2018 · 10 comments · Fixed by #2975
Closed

[v2]How do I add a menubuffer by version2 ? #2965

wszgxa opened this issue Aug 23, 2018 · 10 comments · Fixed by #2975

Comments

@wszgxa
Copy link

wszgxa commented Aug 23, 2018

I am upgrading the react-select to v2 in our project.

In v1 I have used menuBuffer to make menu scroll extra 76px when you open the menu. Because there is a button in bottom, it's position: absolute. If I don't do that, the bottom button will hide the scroll menu.

For now, The upgrading doc suggest to use the new Components API, I have tried that. Not work..

The version I used is 2.0.0.
ezgif com-video-to-gif

@wszgxa wszgxa changed the title How do I add a menubuffer by version2 ? [v2]How do I add a menubuffer by version2 ? Aug 23, 2018
@wszgxa
Copy link
Author

wszgxa commented Aug 23, 2018

For now, I can think a way is to scroll up by myself...

@wszgxa
Copy link
Author

wszgxa commented Aug 23, 2018

I have done a spike for this one. I think the components api can't resolve this issue.

If I want add a gap between menu and the viewport, I need to add a margin or padding.

If I add padding, it will add white space in the menu container.
If I add margin, it will not work. Because we use getBoundingClientRect to get the offsetheight. https://github.com/JedWatson/react-select/blob/master/src/components/Menu.js#L85

But it will not include the margin! https://developer.mozilla.org/en-US/docs/Web/API/CSS_Object_Model/Determining_the_dimensions_of_elements

So I think the best way to address this issue is providing the menuBuffer api.

I will create a PR. If there is another good solution, I will close the PR.

@jossmac
Copy link
Collaborator

jossmac commented Aug 27, 2018

@wszgxa I think you can you achieve the same result using the components API, something like:

import { components } from 'react-select';

const MyMenu = (props) => (
  <div style={{ paddingBottom: 76 }}>
    <components.Menu {...props} />
  </div>
);

Does that work for you?

@wszgxa
Copy link
Author

wszgxa commented Aug 27, 2018

Hi, @jossmac

I have tried that before.
It will break the style and not work correctly, you can see this example. I have tried margin and padding before, and try to add it to the menu and menu-list.

And screenshot in our project:
screen shot 2018-08-27 at 1 55 53 pm

@jossmac
Copy link
Collaborator

jossmac commented Aug 27, 2018

Okay, I see. It's a bit different to most of the primitives.

@wszgxa
Copy link
Author

wszgxa commented Aug 27, 2018

Hi, @jossmac @gwyneplaine

I have seen your comment on the PR.

menuBuffer only makes sense when menuShouldScrollIntoView is true.

Yep, it's bad smell.
I can provide 2 other options to fix this one:

Option 1:

Change menuShouldScrollIntoView to be an object, maybe use the old name scrollMenuIntoView . And this API receive 2 parameters:

type MenuScroll = {
  shouldScrollIntoView: boolean,
  menubuffer?: number,
};

👍 Will be more clear for the relationship between shouldScrollIntoView and menubuffer
👎 Nested object props is complex

Option 2:

getBoundingClientRect will not include the margin and the border, so I add a margin to menu not work.
Adding a margin to Menu should be work, if we include the menu's margin in here

So we can use the components and styles API to add margin to Menu, and include the margin in the scroll calculate.
👍 We don't need to add a new API or change old API
👎 Someone have the same issue should read this one..

Which one do you prefer ?

@wszgxa
Copy link
Author

wszgxa commented Aug 28, 2018

Hi, friend

Do you have the plan to release a version for this 2.0.1? 😆

@jossmac
Copy link
Collaborator

jossmac commented Aug 28, 2018

Hi @wszgxa,

That's the plan, yes.

The changes made in #2975 weren't trivial though, so we're going to get some more eyes on it. The introduction of a render prop inside core Select could have unexpected side effects for some consumers, especially those running tests with enzyme.

@wszgxa
Copy link
Author

wszgxa commented Sep 3, 2018

Hi, mate

Any chance to have a beta release or minimum version release?

This the last issue we don't want to fix by us...

@jossmac
Copy link
Collaborator

jossmac commented Sep 3, 2018

I don't have release permissions; need sign off from @JedWatson / @gwyneplaine

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

Successfully merging a pull request may close this issue.

2 participants