-
Notifications
You must be signed in to change notification settings - Fork 162
Providing pre-built components #172
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
Conversation
Hi there, thanks for the PR. You are correct about the maintenance issues, and I do think that the current state allows for easer maintenance, but as you point out yourself the code to make these components is so minimal, I don't think it adds a lot to provide them (saves a line or two of code) but it also obscures the underlying implementation which is useful to folks in and of itself. i'd say this may be unneeded complexity but others may disagree :)
Thanks for the PR! Yeah – I don't think the pre-built components add too much, and it's just a pain to keep things in sync and handle PRs for adding/removing new components. Maybe some HoC-ification might make sense, but as currently implemented, I don't think we want to add these components.
Thanks for your input. The only real vouch I had for adding the components came from the fact that it seems really likely that most users of the library will implement those components in this simplest form. In fact that is why they need the library and therefore the very first thing they might look for in the documentation are things like ButtonLink or NavItemLink.
But then your arguments are reasonable. Between having all user implement the same exact wrapper or provide them with a simple implementation well enough for their needs I was wondering what was the most reasonable choice. So I will keep it separate and close the PR.
I have seen by looking in the repository history that the library used to provide pre-built components for the most common Bootstrap navigation component (routing tied control). I understand that at the time React Router did not allow to have a wrapper like
LinkContainer, thus requiring parallel maintenance on individual components.Providing pre-built components seems like a good idea as it perfectly matches with the existing paradigm of React Router
Link. I assume that they were previously removed from the library because of the maintenance complexity but maybe I am wrong. Given that we now have a stable and more abstract implementation in the form ofLinkContainer, it is now possible to implement those pre-built components in the form of thin wrappers. I added implementation forButtonLink,NavItemLink,MenuItemLinkandListGroupItemLink. Their implementation is very boilerplate and parallel, thus maintenance on those would be in fact minimal as the abstraction now lies onLinkContainer.For example here is what
ButtonLinkimplementation looks like:Also I must mention that those are still allowing the
IndexLinkbehavior by passing theonlyActiveOnIndexas a property.