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

pass Popper modifiers to DropdownMenu #929

Merged
merged 7 commits into from
Mar 29, 2018

Conversation

borm
Copy link
Contributor

@borm borm commented Mar 27, 2018

No description provided.

attrs.modifiers = !flip ? noFlipModifier : undefined;
attrs.modifiers = !flip ? {
...noFlipModifier,
...modifiers,
Copy link
Collaborator

@virgofx virgofx Mar 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want no-flip ... should we reverse the order here? That way if one specifies as such:

flip={false} modifiers={{flip:{enabled:true}}}

The flip will win out on the modifiers. Rare case, just trying to think which should win?

@virgofx
Copy link
Collaborator

virgofx commented Mar 27, 2018

Looks like one of the tests broke as well. Will need an additional test as well to test different modifier situations ... and handle the expected case for: flip={false} modifiers={{flip:{enabled:true}}}

@borm
Copy link
Contributor Author

borm commented Mar 27, 2018

flip={false} modifiers={{flip:{enabled:true}}}

reverse the order should fix this case

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the docs to add the new prop. Also this needs a few new tests, nothing major.
Take a look at #710

className: PropTypes.string,
cssModule: PropTypes.object,
};

const defaultProps = {
tag: 'div',
flip: true,
modifiers: {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need a default, undefined is fine, even with the spread operator.

@@ -157,7 +157,7 @@ describe('DropdownMenu', () => {
}
);

expect(wrapper.find(Popper).prop('modifiers')).toBe(undefined);
expect(wrapper.find(Popper).prop('modifiers')).toEqual({});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the default prop setting it to and object, this should be able to be reverted

@borm
Copy link
Contributor Author

borm commented Mar 27, 2018

i don't know why, but i can't start project on local env, something wrong with CopyWebpackPlugin i think, or maybe anything else

borms-iMac:reactstrap borm$ node -v
v4.6.2
borms-iMac:reactstrap borm$ npm -v
2.15.11

yes, i know, i have used old version of node.js)))

image

and missed two packages on devDependencies
"babel-preset-es2015" & "babel-plugin-transform-es2015-modules-commonjs"

@TheSharpieOne
Copy link
Member

localhost vs 127.0.0.1? IIRC webpack dev server checks the host header and will not serve content if it doesn't match the settings.

@borm
Copy link
Contributor Author

borm commented Mar 28, 2018

localhost also not worked
image

<div className="docs-example">
<Row>
<Col>
<Dropdown>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need isOpen and toggle props, or you can use UncontrolledDropdown here (and just keep Dropdown with some dummy isOpen and toggle props in the raw example code which is displayed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg, okay)

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