Don't call top-nav button's run function if disabled

[ycombinator]

When the disabled functionality for top nav buttons was created, it was only tested with top nav buttons that provide a template. It was not tested with top nav elements that provide a run function. Consequently, it turned out that the run function was being executed when its top nav button was clicked, even if that button was disabled, which is a bug.

This PR fixes that bug.

UPDATE: It also makes the toggling of enabled/disabled state a function of model changes, not just route/page reload.

[markharwood]

LGTM, thanks @ycombinator

[ycombinator]

Unfortunately using ng-disabled doesn't quite work because it prevents the tooltip on the button from being shown when the button is in a disabled state. I'm working on a better fix now.

[ycombinator]

@markharwood This is ready for review again. Thanks!

[markharwood]

It looks not to trigger the action when clicked but being super-picky it is less "disabled" in appearance/behaviour - it still accepts focus whereas the previous commit didn't.

[ycombinator]

it is less "disabled" in appearance/behaviour - it still accepts focus whereas the previous commit didn't.

Yeah, this is true. Let me see if I can achieve both objectives - make the element not accept focus but also render the tooltip when disabled .

[ycombinator]

@markharwood I made a CSS change to prevent disabled buttons from appearing to accept focus, while also preserving the ability for tooltips to be shown on disabled buttons. Ready for your review again :)

[markharwood]

Regardless of the styling I have a general suspicion the disableButton function of a menu objected is only evaluated on page load.
If I have a declaration like this in the menu declaration:

disableButton: function () {return $scope.selectedIndex === null;},

it doesn't toggle menu states when selectedIndex changes. If I add this plain button to the same page:

      <button  ng-disabled="selectedIndex===null">
        <span >Debug</span>
      </button>

..it clearly does enable/disable with changes to the model.

[ycombinator]

Hi @markharwood, I chatted with @cjcenizal about the styling and whether we should use ng-disabled here. And I see your latest comment about the disabled() function not being evaluated frequently enough as well.

Unfortunately we can't really use ng-disabled as it disables all interactions with the element. While this is desirable for preventing the click and toggling the disabled state on changes to the model it also, unfortunately, prevents the tooltip from appearing, which is functionality we want in some use cases (e.g. to disable the Reporting button on the Discover page due to an incompatible license). Per CJ this is a common issue and the solution would be to disable the click in the click handler function + use styling to make the button appear disabled instead of using ng-disabled.

I also discussed with CJ your point about not being able to tab through disabled buttons. I think removing the outline CSS rule will fix that issue.

So I'm going to do this next:

• Fix the CSS to remove the outline rule.
• Look into how we could evaluate disabled() when the model changes, and not just on page/route reload.

[ycombinator]

@cjcenizal @markharwood I've made style + some code changes per CJ's suggestions and I've also implemented the capability for button enabled/disabled state to be responsive to Angular model changes per Mark's findings. This is ready for review again.

[cjcenizal]

Beautiful! LGTM.

[markharwood]

LGTM too