Add nav prop to Dropdown#636
Merged
TheSharpieOne merged 4 commits intoreactstrap:masterfrom Nov 6, 2017
supergibbs:consolidate-navdropdown
Merged
Add nav prop to Dropdown#636TheSharpieOne merged 4 commits intoreactstrap:masterfrom supergibbs:consolidate-navdropdown
TheSharpieOne merged 4 commits intoreactstrap:masterfrom
supergibbs:consolidate-navdropdown
Conversation
Move NavDropdown tests to Dropdown Remove Uncontrolled NavDropdown and tests Update docs
Collaborator
Author
|
I updated the tests but I can't seem to run them locally. I get the following: Any ideas? Running on a MacBook Pro, NodeJS v6.11.4, npm v5.5.1 and a fresh |
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. |
Closed
TheSharpieOne
requested changes
Oct 21, 2017
Member
TheSharpieOne
left a comment
TheSharpieOne
left a comment
There was a problem hiding this comment.
Looks good. Thanks for this. Just a few little things. If you cannot get to them, I can do it (let me know).
| "Jason Sturges <[email protected]> (https://github.com/jasonsturges)", | ||
| "Jean-Elie Barjonet (https://github.com/jebarjonet)", | ||
| "Jeff Francisco (https://github.com/j-francisco)", | ||
| "Jesse Mandel (https://github.com/supergibbs)", |
Member
There was a problem hiding this comment.
I think this is generated, but eh.
| UncontrolledAlert, | ||
| UncontrolledButtonDropdown, | ||
| UncontrolledDropdown, | ||
| UncontrolledNavDropdown, |
Member
There was a problem hiding this comment.
Would be nice to deprecate this the same as NavDropdown so that it is not a breaking change.
| import NavbarToggler from './NavbarToggler'; | ||
| import Nav from './Nav'; | ||
| import NavItem from './NavItem'; | ||
| import NavDropdown from './NavDropdown'; |
Member
There was a problem hiding this comment.
You still need to import this and export it so that the deprecated component can still be used (and give people the warning).
Collaborator
Author
|
I am happy to make those changes but I am traveling for a couple weeks so
if you want to merge it in before feel free to work on it.
…On Sat, Oct 21, 2017 at 5:06 PM Evan Sharp ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Looks good. Thanks for this. Just a few little things. If you cannot get
to them, I can do it (let me know).
------------------------------
In package.json
<#636 (comment)>:
> @@ -64,6 +64,7 @@
"Jason Sturges ***@***.***> (https://github.com/jasonsturges)",
"Jean-Elie Barjonet (https://github.com/jebarjonet)",
"Jeff Francisco (https://github.com/j-francisco)",
+ "Jesse Mandel (https://github.com/supergibbs)",
I think this is generated, but eh.
------------------------------
In src/Uncontrolled.js
<#636 (comment)>:
> const UncontrolledTooltip = components.UncontrolledTooltip;
export {
UncontrolledAlert,
UncontrolledButtonDropdown,
UncontrolledDropdown,
- UncontrolledNavDropdown,
Would be nice to deprecate this the same as NavDropdown so that it is not
a breaking change.
------------------------------
In src/index.js
<#636 (comment)>:
> @@ -6,7 +6,6 @@ import NavbarBrand from './NavbarBrand';
import NavbarToggler from './NavbarToggler';
import Nav from './Nav';
import NavItem from './NavItem';
-import NavDropdown from './NavDropdown';
You still need to import this and export it so that the deprecated
component can still be used (and give people the warning).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#636 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABDhm8bqFywuILJ0Xyezb8iv3upHwnfVks5sughmgaJpZM4P5gV3>
.
|
export deprecated components
Collaborator
Author
|
@TheSharpieOne I'm back and made those changes! Let me know if there is anything else |
TheSharpieOne
approved these changes
Nov 6, 2017
Member
|
Thanks @supergibbs for making those changes. I was hoping to get around to it but have been too swamped. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidate NavDropdown with Dropdown
Move NavDropdown tests to Dropdown
Remove Uncontrolled NavDropdown and tests
Update docs
#326 (comment)