feat(tab): allow active index string #5713
Conversation
|
There are two other isNumber checks in the tabs component which did not have isString checks added from the pull request I started with. @wesleycho, @RobJacobs did we need to add some is string checks there as well? |
| @@ -85,7 +85,7 @@ angular.module('ui.bootstrap.tabs', []) | |||
|
|
|||
| function findTabIndex(index) { | |||
| for (var i = 0; i < ctrl.tabs.length; i++) { | |||
| if (ctrl.tabs[i].index === index) { | |||
| if (ctrl.tabs[i].index === +index) { | |||
There was a problem hiding this comment.
This change was not in the pull request I started from, but it seemed needed.
There was a problem hiding this comment.
This seems like a strange change - do you know why this is as you have it?
There was a problem hiding this comment.
This was getting called from line 76 where we were now allowing the value to be a string.
The comparison wouldn't work for 1 === "1".
There was a problem hiding this comment.
But why is the type getting mutated?
Allow Tab index to be a string Closes angular-ui#5687 Closes angular-ui#5577 Closes angular-ui#5713
|
Here is the plunker forked with the fix I haven't been following along really, but I'm not sure it is actually working. Was the previous PR ready to go besides test? I gotta run, but I can take a look closer tomorrow. |
|
@RobJacobs, sweet thanks for doing that! |
|
@wesleycho, @RobJacobs, is everyone good with Rob's solution in the commit referenced on this PR? If so I can incorporate his changes into this PR to get this closed. |
|
@deeg Like I mentioned, in the uibTab directive - link function where we try and add an index if none is defined needs to be re-worked. Right now, it is assumed the indexes are numbers, but we may want to generate a random string (4 char?) and check that it is not already assigned to another tab index. Or if you can think of a better way? |
|
@Foxandxss, @wesleycho, i don't want to lose momentum on this PR - do you guys have any thoughts to add here regarding dynamic ID generation? We do this already in other components (datepicker and accordion). |
|
+1 |
|
@mathielen please do not just +1 issues. It adds no intrinsic value and only clutters up the thread. If you would like to show your support for an issue please use GitHub's emoji feature. It's the smiley icon next to the pencil in the upper right corner. This is a work in progress and is nearly complete. |
| @@ -168,6 +168,16 @@ describe('tabs', function() { | |||
| expect(titles().length).toBe(scope.tabs.length); | |||
| expectTabActive(scope.tabs[2]); | |||
| }); | |||
|
|
|||
| it("should watch active state", function() { | |||
There was a problem hiding this comment.
Should use single quotes here
|
@wesleycho, this PR wasn't working. I can make the changes you asked for and incorporate @RobJacobs's changes which are better and actually work, but there was still one outstanding question. I believe it was about the auto generation of the index. Barring that answer, I don't mind cleaning all this up and getting it ready to go. |
|
@deeg, were you still working on a fix for this or did we abandon this feature? |
|
@icfantv I thought I was still waiting on a decision. I'm happy to wrap this up, but @RobJacobs had a branch with a better fix and a question, and I thought we were waiting for feedback. |
|
oh. hmmm. ok. @RobJacobs do you recall what, if anything, we're waiting for? thanks. |
|
Just wanted some feedback on what would be the best approach for the indexes the directive auto generates. Use case is if no index is defined on the tab, the tabset will never set it to active. Previously, the auto generated tab index made the assumption the index would be a number and found the max number in the index collection and added 1. Now with index as a string, that won't work... |
|
Sorry I didn't mean to close this, I deleted the branch by mistake. Will re-open once we decide on a path forward. |
|
it might be too late for this, but in the ng2 port we're going to use the actual tab reference as the "index" - what do you think about that instead or in addition to? |
Add captions to the text, title, table and datatable widgets with a link to a new Filters section in the help popup. This required: - upgrading to angular ui bootstrap 1.3.3 to be able to set the focus on a specific tab (see angular-ui/bootstrap/pull/5713) - converting openHelp function to a directive - adding a new directive to support angular directives and not just HTML in the captions Covers [BS-16403](https://bonitasoft.atlassian.net/browse/BS-16403)
Add captions to the text, title, table and datatable widgets with a link to a new Filters section in the help popup. This required: - upgrading to angular ui bootstrap 1.3.3 to be able to set the focus on a specific tab (see angular-ui/bootstrap/pull/5713) - converting openHelp function to a directive - adding a new directive to support angular directives and not just HTML in the captions Covers [BS-16403](https://bonitasoft.atlassian.net/browse/BS-16403)
Allow Tab index to be a string
Closes #5687
Closes #5577