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(Table): add borderless option #956

Merged
merged 2 commits into from
Apr 16, 2018

Conversation

frontsideair
Copy link
Contributor

@frontsideair frontsideair commented Apr 15, 2018

This option appeared in bootstrap@v4.1.0

Looks like we need to bump bootstrap devDependency to 4.1.0. Is it OK if I do this in the PR? And bootstrap doesn't seem to be a peerDependency. Should I open an issue?

This option appeared in bootstrap@v4.1.0
@RodolfoSilva
Copy link

You was most faster than me. I was planning to send this PR. Thanks @frontsideair 😄

@TheSharpieOne
Copy link
Member

Thanks for the PR. As for the deps, this project doesn't depend on bootstrap for anything outside of the docs, which is why it's a devDep. We'll need to bump the devDep to ensue the docs are built with that version to showcase this new ability.

@frontsideair
Copy link
Contributor Author

That's why I suggested peer dependency, as any user of this project will need to install Bootstrap and this needs to be within certain bounds otherwise the package manager should show a warning.

@TheSharpieOne
Copy link
Member

They don't actually need bootstrap to use this. There are no references to bootstrap in the code (no imports). That is not what peerDeps are for, it mostly when you depend on something but want to ensure that the version used is used for all of the things which also depend on it; to avoid loading multiple versions. This is why react is a peerDep in libraries and why prop-types is not.

@frontsideair
Copy link
Contributor Author

Thanks for the explanation, you are correct. I just wanted to make sure the users would not be confused if they installed bootstrap@v4.0.0 and saw borderless prop not working. (As is the case in the netlify build.) I guess a notice in documentation should suffice.

Should I bump do the bump in this PR, or another one, or do you have another strategy for the bump?

@TheSharpieOne
Copy link
Member

Yes, please bump with this since this adds the example to the docs and without the bump the example in this PR doesn't work as intended.
To avoid the issue of users not having the right version of bootstrap, we may want to just indicate in the docs the minimum bootstrap version (per feature) somehow. Idk just a thought.

@frontsideair
Copy link
Contributor Author

Done.

Adding docs per component sounds good enough to me.

@TheSharpieOne TheSharpieOne merged commit 210b53f into reactstrap:master Apr 16, 2018
@frontsideair frontsideair deleted the table-borderless branch April 16, 2018 14:04
@GHEMID-Mohamed
Copy link

This still not working, did you guys fix it ?

@frontsideair
Copy link
Contributor Author

You have to be using Reactstrap 6.0.0 or greater and Bootstrap 4.1.0 or greater. Can you confirm that?

@GHEMID-Mohamed
Copy link

"bootstrap": "^4.1.0", 
"reactstrap": "^5.0.0",

@frontsideair
Copy link
Contributor Author

6.0.0 seems to be released two days ago, that's the version this PR was shipped.

@GHEMID-Mohamed
Copy link

Oh Great, I'll test it right now 👍 Thank you

@GHEMID-Mohamed
Copy link

@frontsideair it works perfectly 🥇

@TheSharpieOne
Copy link
Member

TheSharpieOne commented May 3, 2018

Sorry about the major version bump, I had introduced a breaking change which caused that.
In the future I'm going to try manage multiple major versions so that fixes and features are applied to multiple major releases... Or something like that.

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

4 participants