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

Add nav prop to Dropdown #636

Merged
merged 4 commits into from
Nov 6, 2017
Merged

Add nav prop to Dropdown #636

merged 4 commits into from
Nov 6, 2017

Conversation

supergibbs
Copy link
Collaborator

Consolidate NavDropdown with Dropdown
Move NavDropdown tests to Dropdown
Remove Uncontrolled NavDropdown and tests
Update docs

#326 (comment)

Move NavDropdown tests to Dropdown
Remove Uncontrolled NavDropdown and tests
Update docs
@supergibbs
Copy link
Collaborator Author

supergibbs commented Oct 14, 2017

I updated the tests but I can't seem to run them locally. I get the following:

➜  reactstrap git:(consolidate-navdropdown) ✗ npm test 

> reactstrap@5.0.0-alpha.3 test /Users/jmandel/Development/supergibbs/reactstrap
> cross-env BABEL_ENV=test react-scripts test --env=jsdom

2017-10-14 12:48 node[8340] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2017-10-14 12:48 node[8340] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: Error watching file for changes: EMFILE
    at exports._errnoException (util.js:1020:11)
    at FSEvent.FSWatcher._handle.onchange (fs.js:1421:9)
npm ERR! Test failed.  See above for more details.

Any ideas? Running on a MacBook Pro, NodeJS v6.11.4, npm v5.5.1 and a fresh npm install after removing node_modules/lock file

@TheSharpieOne
Copy link
Member

I get that on my work Mac as well. I am able run coverage which does the test but does not watch the files. Takes a little longer to run, but at least it works.

@supergibbs supergibbs mentioned this pull request Oct 15, 2017
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.

Looks good. Thanks for this. Just a few little things. If you cannot get to them, I can do it (let me know).

@@ -64,6 +64,7 @@
"Jason Sturges <jason@jasonsturges.com> (https://github.com/jasonsturges)",
"Jean-Elie Barjonet (https://github.com/jebarjonet)",
"Jeff Francisco (https://github.com/j-francisco)",
"Jesse Mandel (https://github.com/supergibbs)",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is generated, but eh.

const UncontrolledTooltip = components.UncontrolledTooltip;

export {
UncontrolledAlert,
UncontrolledButtonDropdown,
UncontrolledDropdown,
UncontrolledNavDropdown,
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to deprecate this the same as NavDropdown so that it is not a breaking change.

src/index.js Outdated
@@ -6,7 +6,6 @@ import NavbarBrand from './NavbarBrand';
import NavbarToggler from './NavbarToggler';
import Nav from './Nav';
import NavItem from './NavItem';
import NavDropdown from './NavDropdown';
Copy link
Member

Choose a reason for hiding this comment

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

You still need to import this and export it so that the deprecated component can still be used (and give people the warning).

@supergibbs
Copy link
Collaborator Author

supergibbs commented Oct 22, 2017 via email

@supergibbs
Copy link
Collaborator Author

@TheSharpieOne I'm back and made those changes! Let me know if there is anything else

@TheSharpieOne TheSharpieOne merged commit 48edd6b into reactstrap:master Nov 6, 2017
@TheSharpieOne
Copy link
Member

Thanks @supergibbs for making those changes. I was hoping to get around to it but have been too swamped.

@supergibbs supergibbs deleted the consolidate-navdropdown branch November 7, 2017 05:46
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

2 participants