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

feat(tabs): use uib- prefix #4449

Closed
wants to merge 4 commits into from
Closed

Conversation

icfantv
Copy link
Contributor

@icfantv icfantv commented Sep 23, 2015

No description provided.

@Foxandxss
Copy link
Contributor

Few things. No backquotes on title.

The demo html needs to be updated to use the prefix and the readme if mentions (and it does in this case) need to be updated with prefixes as well.

The main demo app has tabs so that needs to be fixed (I can do that in a separate commit, no problem).

My biggest problem here is the <tab-heading>. We have two directives with this type of feature (accordion and this) and they both behave differently.

In Accordion we have our own directive for that and for this one, we have sort of a hack. My point in here is that in here we don't have <tab-heading> prefixed.

So I think that we need to prefix that as well but also supporting it without prefix for the deprecated one. Thoughts on this @wesleycho ?

@icfantv
Copy link
Contributor Author

icfantv commented Sep 23, 2015

@Foxandxss, ok. the title can be fixed on the merge. demo has been updated. tab-heading was updated to contain the uib-prefix and all relevant tests were updated as well. per our discussion, it NOT made into a directive at this time.

};
}])

.directive('tabHeadingTransclude', ['$log', '$tabSuppressWarning', function($log, $tabSuppressWarning) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, forgot to mention that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. there were a couple of lines that needed indent fixes.

@icfantv
Copy link
Contributor Author

icfantv commented Sep 24, 2015

I think my comment got lost. Fixed indentation in a couple of spots.

@wesleycho wesleycho added this to the 0.14.0 (Bootstrap 3.3) milestone Sep 24, 2015
@wesleycho
Copy link
Contributor

This LGTM - @Foxandxss ?

@icfantv
Copy link
Contributor Author

icfantv commented Sep 24, 2015

Hang on, there's some duped lines in the second test. I'll remove and repush.

@icfantv
Copy link
Contributor Author

icfantv commented Sep 25, 2015

The template was getting compiled and digested twice which was resulting in double the number of $log warning messages than expected. This is fixed.

@icfantv icfantv closed this in d25a8c2 Sep 26, 2015
@Foxandxss
Copy link
Contributor

Alrighty merged. Changed $tabSuppress to $tabsSuppress and there was a missing prefix on the demo.

@icfantv icfantv deleted the tabs_prefix branch October 6, 2015 21:18
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