Adding condition support to tabs

[kyse]

Adding condition support for tabs:

• Tab visibility via form definition condition option.
• Tab ng-if vs ng-show via form definition tabCondition, and tabPaneCondition options.

[Anthropic]

@kyse, if you have a chance, a demo or example would help me with evaluating this, there may be a way to avoid new attribute names perhaps... tab visibility is great though thank you for looking at it, not sure why this wasn't in ASF already.

[Anthropic]

Oh and I really appreciate your responding to some other issues, every bit of help is greatly appreciated :)

[kyse]

Not sure what you mean by avoiding a new attribute. Only new attribute is ng-if or ng-show on the tab li element to handle the hiding or destroying of the tab element and adding the ability to switch ng-show to ng-if on the tab-pane element. This gives the developer full control over whether or not controls are hidden or destroyed. For cases like ui-select multi-select where it needs to be rendered on the first tab or css sizing breaks, ng-if is needed, and one would want to use destroyStrategy of 'retain' rather than 'empty'.

[Anthropic]

@joelwkent can you review this since you have worked on a tabs PR recently?
@kyse could you make a protractor test for this change? Sounds like a nice change to have an be great to get it merged if it works but also ensure we don't break it when the core changes.

[joelwkent]

Thanks for the PR and apologies for my delay in taking a look.

I agree, I'm also weary of adding the new attributes 'tabCondition' and 'tabPaneCondition' as updates to the API should be carefully considered. I would expect conditions on tabs to work similar to sections:

[
  "name",
  {
    "type": "tabs",
    "condition": "model.showAllTabs == true",
    "tabs":
    [
      {
        "title": "Tab 1",
        "items": [
          "nick",
          "alias"
        ]
      },
      {
        "title": "Tab 2",
        "condition": "model.showTabTwo == true",
        "items": [
          "tag"
        ]
      }
    ]
  }
]

Conditions on type 'tabs' (in my example above "condition": "model.showAllTabs == true") are already supported by ASF. But conditions on individual tabs (in my example above "condition": "model.showTabTwo == true") are not currently supported by ASF.

It looks like this PR adds:

• 'tabPaneCondition' attribute which toggles the div.tab-pane from using ng-show to ng-if
• 'tabCondition' attribute which toggles the the tabs (li tags) from using ng-show to ng-if
• also adds conditions for individual tabs by hiding/removing the tab (li tag) but not the tab-pane

Have I understood the PR correctly?

I have just tested this out locally by updating the form definition as follows:

[
  "name",
  {
    "type": "tabs",
    "condition": "model.showAllTabs == true",
    "tabCondition": "if",
    "tabPaneCondition": "not if",
    "tabs":
    [
      ...
]

and the behaviour is as I listed above.

There is one bug with this implementation. That is if the first tab has a condition that is false then the tab (li tag) will be hidden but the tab-pane will still be shown.

It is also worth testing that tabPaneCondition (both 'if' and 'not if') does not conflict with destroyStratergy.

I'm struggling to think of situations where you'd want this level of control but that does not mean that others do not want this. Can you give examples of where you would want to set hide/remove of the tab and separately set the hide/remove of the tab-pane? This could help us agree on suitable API changes.

I'm a little cautious as I'm not aware of any other part of ASF allowing the user to switch from remove (ng-if) to hide (ng-show) for example tab array, conditions etc...

[kyse]

Adding new attributes, as you discovered later in your post, there are tabs and there are tab-panes. If you set tab-panes to ng-if vs ng-show, as I pointed out before you better make sure your destroy strategy meets your needs because otherwise the values will be stripped off your model when you switch tabs. However, I ran into a case where I needed a specific control that refused to work (css wise) unless it was on the first tab. Something about css and ng-show hidden elements screwing up the programatic css of that control led me to the thought of using ng-if on the tab-panes. Whether or not the tabs themselves need it, who knows. I dont care, but the option is there.

As far as being able to use ng-if or ng-show on individual tabs, that's just common sense in my book to allow folks to enforce some kind of workflow on their form.

I find it funny your concerned with adding new features to your library, yet when someone needs a new feature they have to wait 30+ days for you to review/consider adding it so they can use unmodified vendor libraries in their apps. Just a thought. I'm dev'ing for deadlines, trying to offer PR's is almost suicide time wise. I have plenty I can offer, but whats the point? I practically have my own decorator and custom schema forms vendor library now.

[joelwkent]

@kyse I hope you don't take offense, none was intended, when I referred to being cautious it is because we have a substantial number of users and we have to be careful how we grow, we don't have enough test cases yet and we are in the middle of refactoring the code, we love these sorts of contributions and what we usually try to do is start a conversation about how to fit any change into the framework by also considering how to make it future proof.

We are just like you, we use the library in our day to day jobs and contribute our own time after hours, so believe me, we know how much effort it takes. And we really appreciate yours!

We are hoping to have more time this weekend to work on the repos, once the bulk of the core work is done we will start to work more on the decorators, we all just keep having life get in the way! We need help from people like you, but we also need second opinions as none of us have time to review everything there's just too many users and tickets and we don't have enough tests to feel safe that we aren't breaking something.

[Anthropic]

@kyse did you have a solution to the issue "That is if the first tab has a condition that is false then the tab (li tag) will be hidden but the tab-pane will still be shown.", I tried to get this to work myself but realised that there are issues with the way the bootstrap tabs handles selection, I was thinking it may need an additional controller to manage selected index?

[kyse]

Looks like the issue is here: ng-init=\"selected = { tab: 0 }\".

Honestly, you guys already use a custom builder for Tabs. So just throw a ng-controller on the tabs div there. Like you concluded, you'll need a controller (or directive link fn) to grab scope to make use of your eval functions to resolve the conditions. Don't see why adding a short controller to the mix in the bootstrap-decorator would be too big of a deal since you're already adding another builder. You'll need a $timeout to allow a digest cycle so the tabs scope gets created before running through the condition evals though. I have my tabs linked to my router, so I do notice a slight delay on switching to the first proper tab it should show, which sucks. But perhaps that wouldn't be an issue under normal circumstances.

In the Tabs builder:

    var container = args.fieldFrag.querySelector('.schema-form-tabs');
    container.setAttribute('ng-controller', 'SfTabsInitController as sfTabsInit');

And at the bottom of the file tack on a controller:

.controller('SfTabsInitController', ['$scope', '$timeout',
    function ($scope, $timeout) {
        $timeout(function() {
            if (!$scope.form.tabs.some(function(tab, index) {
                if (!angular.isDefined(tab.condition) || $scope.evalExpr(tab.condition, { model: $scope.model, arrayIndex: index })) {
                    $scope.selected.tab = index;
                    return true;
                }
                return false;
            }))
                $scope.selected.tab = 0;
        });
    }
]);

Probably smart to keep the ng-init for selected.tab property on scope there then you can add some conditional check to applying the controller if you so desire.

Edit: setting selected.tab when index = 0 returned falsey, updated to correct filter returning truthy.