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

feat(tabs): Allow setting optional classes on each tab. #5430

Closed
wants to merge 1 commit into from

Conversation

amahfouz
Copy link
Contributor

@amahfouz amahfouz commented Feb 7, 2016

Allow setting any set of optional CSS classes on a uib-tab element.

This is to allow specifying classes such as "btn-sm" for tab pills.

@@ -34,16 +34,18 @@ describe('tabs', function() {
scope.first = '1';
scope.second = '2';
scope.actives = {};
scope.classesFirst = "first-classes-1";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better named firstClass - also first-class-1, and use single quotes

@icfantv
Copy link
Contributor

icfantv commented Feb 7, 2016

This also needs documentation and demo updates.

@amahfouz
Copy link
Contributor Author

amahfouz commented Feb 9, 2016

I fixed all reported issues.

Any update on the status of this?

Thanks.

@icfantv
Copy link
Contributor

icfantv commented Feb 9, 2016

@amahfouz looks like you need to rebase first.

@wesleycho
Copy link
Contributor

Needs squashing and rebasing (squash first).

@amahfouz
Copy link
Contributor Author

amahfouz commented Feb 9, 2016

Thanks for the prompt reply. I had already done the squash.

Now I did: git pull --rebase and did not get any conflicts, but there is nothing to push. What am I missing?

@wesleycho
Copy link
Contributor

git pull --rebase origin master

@amahfouz
Copy link
Contributor Author

amahfouz commented Feb 9, 2016

Got it. Thanks. I have now squashed, rebased, and force pushed.

Rerunning tests now.

@amahfouz
Copy link
Contributor Author

amahfouz commented Feb 9, 2016

Tests now fail :(
Still got work to do.

@amahfouz
Copy link
Contributor Author

amahfouz commented Feb 9, 2016

Checks now running. Let me know if I need to do anything else.

@@ -132,6 +133,10 @@ angular.module('ui.bootstrap.tabs', [])
});
}

if (attrs.classes === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Angular has built in functions for checking if values are undefined or are defined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to fix this, resquash, rebase, etc. or this is within the margin of tolerance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use angular.isUndefined here

@wesleycho
Copy link
Contributor

This needs to be rebased.

@amahfouz amahfouz force-pushed the master branch 3 times, most recently from f3125d7 to 6311881 Compare February 25, 2016 02:30
@amahfouz
Copy link
Contributor Author

I have made the last requested fix, squashed, and rebased. Please review.

@@ -143,6 +144,10 @@ angular.module('ui.bootstrap.tabs', [])
}
}

if (angular.isUndefined(attrs.classes)) {
scope.classes = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ' instead of "

@amahfouz
Copy link
Contributor Author

Done. Squashed, rebased and pushed.

@wesleycho wesleycho added this to the 1.2.0 milestone Feb 25, 2016
@wesleycho wesleycho closed this in 8364e76 Feb 25, 2016
icfantv added a commit to icfantv/bootstrap that referenced this pull request Feb 29, 2016
* add breaking change note for 1.2.0 release for tab change in angular-ui#5430.
wesleycho pushed a commit that referenced this pull request Feb 29, 2016
* add breaking change note for 1.2.0 release for tab change in #5430.

Closes #5555
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.

4 participants