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

Added closeMenuOnScroll prop #2809

Merged
merged 1 commit into from
Jul 23, 2018
Merged

Added closeMenuOnScroll prop #2809

merged 1 commit into from
Jul 23, 2018

Conversation

Vynlar
Copy link

@Vynlar Vynlar commented Jul 18, 2018

Added a new prop that allows the user to have the menu close when they scroll the window. This is useful when you have a portaled menu inside of a scrollable view. Without this, the dropdown often ends up in strange places.

A few notes:
I was unable to get the npm start server to run, which means I can't run the cypress tests.
Here's the error I got:

defaultProps = convert(p, { ...context, mode: "value" });
SyntaxError: Unexpected token ...

It seems like transform-object-rest-spread isn't being enabled despite it being inside of .babelrc.

I attempted to add unit tests, but since this feature relies heavily on binding listeners to the document object, I was unable to write any that were meaningful. Considering the touchdown et. al. handlers are not covered in the unit tests, I figured it would be reasonable for mine to not be as well.

Flow types + linting check out fine.

If someone is willing to help me get my server to start, I'm happy to add some cypress tests that cover the new functionality.

Great job with the new API, migrating from v1 has been wonderful! I've deleted several hundred lines of code that are no longer necessary because of the new styling/component system.

Keep it up!

@JedWatson
Copy link
Owner

This is a really neat addition, thanks @Vynlar

@JedWatson JedWatson merged commit 8aebc03 into JedWatson:v2 Jul 23, 2018
@ghfkui
Copy link

ghfkui commented Jul 26, 2018

  1. Menu cannot close when set 'closeMenuOnScroll'. Why not exec 'this.onMenuClose'?
  2. If 'stopPropagation' in parent element, listener cannot be fired.

@Vynlar
Copy link
Author

Vynlar commented Jul 26, 2018

@ghfkui Could you provide more detail as to what the problem is? Ideally some example code that demonstrates the issue.

@ghfkui
Copy link

ghfkui commented Jul 27, 2018

Sorry @Vynlar, the second case does not exist.

Case1: Fired the document's scroll, it just exec 'this.props.onMenuClose'. At this time menu is not closed.
I think it should exec 'this.onMenuClose'. Does need to set 'menuIsOpen' in outside?

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 this pull request may close these issues.

None yet

3 participants