Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

style(tabs): Match Bootstrap tabs / pills styles #5202

Closed
wants to merge 1 commit into from
Closed

style(tabs): Match Bootstrap tabs / pills styles #5202

wants to merge 1 commit into from

Conversation

philBrown
Copy link
Contributor

Adds all style declarations for Bootstrap (v3.3.6) tabs and pills.

See http://getbootstrap.com/components/#nav

Adds all style declarations for Bootstrap (v3.3.6) tabs and pills.

See http://getbootstrap.com/components/#nav
@wesleycho
Copy link
Contributor

LGTM

@wesleycho wesleycho added this to the 1.0.1 milestone Jan 11, 2016
@wesleycho wesleycho closed this in 1340812 Jan 11, 2016
@philBrown philBrown deleted the patch-1 branch January 11, 2016 06:06
@georgmu
Copy link

georgmu commented Jan 11, 2016

Wouldn't it be better to alter the template instead of the demo?

This way, everybody has to implement it. Changing the default template from "<div ..." to "<a ..." with an empty href would solve it for everybody. This way it would match the bootstrap stylings.

@wesleycho
Copy link
Contributor

@georgmu that has an issue as well, although I can't remember exactly what it was - it might have to do with the element the browser triggers the click on as the anchor element instead of any nested clickable element.

It was literally a last resort option, we tried multiple attempts, only to find that each approach caused other regressions/issues.

@georgmu
Copy link

georgmu commented Jan 11, 2016

I changed the following and it works fine for me. But if you have other experience: ok.

diff --git a/template/tabs/tab.html b/template/tabs/tab.html
index b7b83e5..d93a2d3 100644
--- a/template/tabs/tab.html
+++ b/template/tabs/tab.html
@@ -1,3 +1,3 @@
 <li ng-class="{active: active, disabled: disabled}" class="uib-tab">
-  <div ng-click="select()" uib-tab-heading-transclude>{{heading}}</div>
+  <a ng-click="select()" uib-tab-heading-transclude href="">{{heading}}</a>
 </li>

Maybe I should raise an issue within bootstrap to not depend on "a" inside of the li element, but support div as well...

The other idea would be to provide the css somewhere in angular-ui-bootstrap. But without this, the out-of-the-box tpls solution is not usable without major css additions.

@philBrown
Copy link
Contributor Author

@georgmu the original issue is here and at the moment, I agree that the best approach is to provide your own CSS or override the template.

Maybe I should raise an issue within bootstrap to not depend on "a" inside of the li element, but support div as well...

I too am not a fan of all the .bootstrap-component > tag > tag stuff in Bootstrap's CSS but I don't like your chances of getting that changed ;)

@wesleycho
Copy link
Contributor

@philBrown take a look at #5211 - I figured out a solution to bundle the CSS, will go out in a few hours when we release 1.0.1 (eta ~6-8 hours from now)

@philBrown
Copy link
Contributor Author

@wesleycho it's just a shame you'll have to keep your CSS up-to-date with any Bootstrap changes. I might try creating a pull request over at https://github.com/twbs/bootstrap to change their CSS to

.nav-tabs > li > a, .nav-tabs > li > .nav-tab

then you'll just need to add a class="nav-tab" to whatever element acts as the trigger.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants