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

feat(Dropdown): setActiveFromChild prop #977

Conversation

FutureKode
Copy link
Contributor

@FutureKode FutureKode commented Apr 24, 2018

See issue: #960

@TheSharpieOne
Copy link
Member

I'm not 100% sure this will work with the use-case in #960. Using react-router's Link with it's activeClassName means that the active prop will not get set, only the "active" class if and when it is active and even that is only evaluated afterwards.
Looking at how react-bootstrap created this feature, they are just matching routes directly in their code to determine if the route is active rather than using anything react-router would give them.
So while this would work, it would not be able to leverage react-router to determine if the link is active, leaving it up to the user to add that logic in the active prop.

@FutureKode
Copy link
Contributor Author

Yes you are right :( it doesn't work when using react-router Link

@TheSharpieOne
Copy link
Member

Still a nice feature. The kinks for the Link integration can be worked out later. For now, people can manage the active state of the dropdown via the items's active props rather than having additional logic in their code.

@TheSharpieOne TheSharpieOne merged commit 1b47757 into reactstrap:master May 1, 2018
@apaatsio
Copy link
Contributor

apaatsio commented May 4, 2018

There's zero documentation for setActiveFromChild. It would be nice to always update the documentation when new features are added.

@FutureKode FutureKode deleted the dropdown-toggle-inherit-active-from-children branch May 4, 2018 12:39
@FutureKode
Copy link
Contributor Author

FutureKode commented May 4, 2018

It is listed in the dropdown props

I could add a section showing how to use it if you think it is worth it

@apaatsio
Copy link
Contributor

apaatsio commented May 4, 2018

I don't think the property name is enough documentation in this case. A simple usage example would be clarifying enough, though.

FutureKode added a commit to FutureKode/reactstrap that referenced this pull request May 10, 2018
@FutureKode
Copy link
Contributor Author

@apaatsio good idea. I have created a PR, let me know if it is okay :)

FutureKode added a commit to FutureKode/reactstrap that referenced this pull request May 11, 2018
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