Adding options cssClassTabsLi and cssClassTabsA #73
Conversation
|
If this pull request gets merged I'd like to submit another one that outlines the steps to use this lib with BS4 in the README. |
package.json
Outdated
| @@ -20,6 +20,7 @@ | |||
| ], | |||
| "dependencies": { | |||
| "bootstrap": "^3.1.1", | |||
| "bootstrap4": "npm:bootstrap@^4.3.1", | |||
There was a problem hiding this comment.
Can you remove this. I wouldn't want to introduce BS4 as a dependency for current users who are only using BS3.
There was a problem hiding this comment.
I can. But it'll break the demo in run. So I'd have to remove it as well.
Maybe the demos should have their own dependencies?
There was a problem hiding this comment.
I moved it to the dev dependencies. How do you like that approach?
also adjusted dependencies: bootstrap 3 || 4 for the lib and bs3 AND bs4 as dev dependencies for the demos
| typeof jQuery.fn.tab.Constructor === 'undefined' || | ||
| typeof jQuery.fn.tab.Constructor.VERSION === 'undefined') { | ||
| throw new Error("jquery-bootstrap-scrolling-tabs requires Bootstrap's tab plugin for jQuery"); | ||
| } |
There was a problem hiding this comment.
Can you please remove this error checking code, both blocks. I trust the developer to have their tabs working correctly before trying to add this plugin. Also, the second block, checking for the tab plugin, as written seems to break some versions of BS3 that don't include the constructor VERSION. Both of the demo plunks threw errors because of that.
There was a problem hiding this comment.
The problem with 3 as default without a means to check if 4 is installed is that "dependencies": { "bootstrap": "^3.1.1 || ^4.3.1", …} will cause the package manager to install bs4 unless the developer installed bs3 before. So I think it's problematic to choose 3 as default while the package manager by default will install 4.
There was a problem hiding this comment.
How about I remove the check if BS exists but try to find jQuery.fn.tab.Constructor.VERSION and if it exists I use this version as default. If it doesn't exist I'll use 3.
|
Ok. Please check my comments and latest changes. I'll leave the PR as is. I'd be happy if you were able to take it from here (shouldn't be much work left to be done). |
See my comment here.
These options make using this nice widget much easier with BootStrap4. I've also included a new example in run to demonstrate and test this.
One thing: Since bower does not support multiple versions of the same dependency (at least I couldn't find out how), I'm afraid the example in run will not work when the libs are installed using bower. Generally I couldn't quite find out how bower is supposed to act in this repo.