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

refactor(datepicker): implement one time bindings #3443

Closed
wants to merge 1 commit into from
Closed

refactor(datepicker): implement one time bindings #3443

wants to merge 1 commit into from

Conversation

RobJacobs
Copy link
Contributor

Reducing the number of watches created for the
datepicker by implementing one time binding
on the datepicker templates. Removing aria-disabled
attribute as the ngAria module will add that
to the button with ng-disabled.

<th><button type="button" class="btn btn-default btn-sm pull-right" ng-click="move(1)" tabindex="-1"><i class="glyphicon glyphicon-chevron-right"></i></button></th>
</tr>
<tr>
<th ng-show="showWeeks" class="text-center"></th>
<th ng-repeat="label in labels track by $index" class="text-center"><small aria-label="{{label.full}}">{{label.abbr}}</small></th>
<th ng-show="::showWeeks" class="text-center"></th>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if one.time on ng-show is gonna work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should be using ng-if, but then tests fail.

@RobJacobs RobJacobs changed the title refactor(datepicker): implement one time bindings refactor(datepicker): implement one time bindings WIP Mar 26, 2015
@wesleycho wesleycho added this to the 0.13.1 milestone Apr 5, 2015
@wesleycho
Copy link
Contributor

Is removing aria-disabled a good idea? Users of ngAria would want that, no?

@RobJacobs
Copy link
Contributor Author

Partially fixes #2613, should compliment #1915

@realityking
Copy link
Contributor

From my understanding of the ARIA spec, the aria-disabled should stay as this indicated that the griddle is disabled, not just the button within it.

BTW, for buttons, ngAria is not needed. All modern browsers map the accessible information correctly for disabled button. Unfortunately ngAria will add unnecessary attributes to the DOM anyway.

@RobJacobs
Copy link
Contributor Author

@wesleycho @realityking I should have done a better job of explaining why I think the aria-disabled binding should be removed on the td element. Here is a plunk with the latest build from master, Angular 1.3, and ngAria loaded. You will notice the rendered html for the day/month/year picker buttons when disabled looks like:

<td aria-disabled="true" ng-repeat="dt in row track by dt.date">
  <button tabindex="-1" disabled="disabled" aria-disabled="true"

Using ng-disabled directive on the button element will cause the ngAria service to add the aria-disabled attribute for you. I don't think it's necessary to have the aria-disabled attribute on both the td and button elements.

The rendered html for an enabled button doesn't have the disabled or aria-disabled attributes at all where the enabled td element aria-disabled attribute is rendered as aria-disabled="false". That leads me to believe the correct implementation for using aria-disabled is to remove or add the attribute versus setting the value.

@RobJacobs RobJacobs changed the title refactor(datepicker): implement one time bindings WIP refactor(datepicker): implement one time bindings Apr 11, 2015
@realityking
Copy link
Contributor

I've spent some time with the ARIA spec, but I'm far from an expert.

Still there are a few things worth clarifying:

  1. Unlike the disabled attribute, aria-disabled (and other aria states) is not a boolean attribute whose presence or absence determines what's going on, the value actually matters. (Otherwise we'd write aria-hidden="aria-hidden"). It's a confusing mess, but that's what you get from an evolving system.
  2. In my opinion, the aria-disabledattribute is correct, because it describes the state of the gridcell, not that of the button within it.

The ARIA Best Practices document has a section on datepicker date picker, it doesn't mention aria-disabled explicitly but the linked implementation in Dojo does use it.

@chrisirhc
Copy link
Contributor

I like the change to remove aria-disabled, and leave it to ngAria to handle that for us since we're moving to AngularJS 1.3 anyway.

If no other issues, I think this PR is good to go for 0.13.1 .

@hamfastgamgee
Copy link
Contributor

I've been running with a closely-related variant of this and #1915 in my local fork for a long time, so I'm going to add a couple of suggestions to the patch if you don't mind. :)

(For reference, we have one data entry page that has probably 50 datepickers on it. Without this and #1915, let's just say that page would not be loading in under 20 seconds...)

@RobJacobs
Copy link
Contributor Author

@hamfastgamgee I decided to err on the side of caution, I can't imagine why someone would want to change the show weeks or start day attributes while the datepicker is visible, but you never now.

_Edit_
After further testing, it seems the datepicker does not support dynamic starting day or show weeks attributes, so I'll make those suggested changes as well. The ng-disabled with 1 time bindings causes the min-date and max-date tests to fail.

@hamfastgamgee
Copy link
Contributor

Fair enough. Might warrant further discussion as to whether those are actually desired, though, since at least in the start date case it's one extra watcher per day visible. (The showWeeks stuff is one per picker, so not as big of an issue.)

@wesleycho
Copy link
Contributor

@RobJacobs what's the state of the PR? Since we released 0.13.0, we are in the position to merge this if this is ready.

@RobJacobs
Copy link
Contributor Author

@wesleycho It's good to go as far as I'm concerned. Rebased and tests still passing.

@wesleycho
Copy link
Contributor

Alright, I'll review it tonight

@BobbieBarker
Copy link
Contributor

I approve

@@ -59,10 +59,10 @@ describe('datepicker directive', function () {
return element.find('thead').find('tr').eq(1);
}

function getLabels() {
function getLabels( dayMode ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces around dayMode

@wesleycho
Copy link
Contributor

LGTM minus one code style comment.

Reducing the number of watches created for the
datepicker by implementing one time binding
on the datepicker templates. Removing aria-disabled
attribute as the ngAria module will add that
to the button with ng-disabled.
@RobJacobs RobJacobs closed this in 2a2e5de May 13, 2015
@RobJacobs RobJacobs deleted the datepicker-watches branch May 13, 2015 17:43
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.

8 participants