Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

refactor(switch): refactor for spec changes #882

Closed
wants to merge 3 commits into from
Closed

Conversation

ajoslin
Copy link
Contributor

@ajoslin ajoslin commented Dec 9, 2014

  • Focus styles
  • Proper theming
  • Pullable/draggable, using our own 65-line dragging implementation that works on windows phone/desktop/android/iOS

PROBLEMS:

  • How to unit test? We don't use click events, we listen to mousedown/mouseup
  • Disable transition of background-color on initial load?

@googlebot
Copy link

CLAs look good, thanks!

@ajoslin ajoslin added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 9, 2014
@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 9, 2014

@robertmesserle do you know a good way to disable the transition of background-color on initial load?

@ajoslin ajoslin force-pushed the wip-switch branch 2 times, most recently from 1f0d1c6 to f9ad10c Compare December 9, 2014 17:23
}
.md-switch-thumb {
&:focus .md-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Switches need interaction styles for mouse, keyboard, and touch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about focus, thanks.

@marcysutton
Copy link
Contributor

ngAria isn't able to compute aria-checked with the use of ngmodel on the second
"Switch 2: nope" component:

$scope.data = {
cb2: 'nope',
}

That data setup appears truthy to ngAria, triggering aria-checked="true" even though the switch isn't active.

@ajoslin ajoslin force-pushed the wip-switch branch 2 times, most recently from 4717cb2 to 01d04e7 Compare December 9, 2014 19:35
@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 9, 2014

Ready for final review and merge.

@marcysutton fixed both problems.

@mzbyszynski
Copy link
Contributor

Hi @ajoslin! 👍 👍 for this PR!

I was wondering, have you figured out how to disable the background-color transition during initial loading yet? If not, you could try doing something similar to what ngCloak does and leave the style class that specifies the transition off of your template and add it in the directive's compile function.

If that doesn't work, you could try doing it the way that ngAnimate does. Here is the relevant bit of code with some excellent code commenting from @matsko

      // Wait until all directive and route-related templates are downloaded and
      // compiled. The $templateRequest.totalPendingRequests variable keeps track of
      // all of the remote templates being currently downloaded. If there are no
      // templates currently downloading then the watcher will still fire anyway.
      var deregisterWatch = $rootScope.$watch(
        function() { return $templateRequest.totalPendingRequests; },
        function(val, oldVal) {
          if (val !== 0) return;
          deregisterWatch();

          // Now that all templates have been downloaded, $animate will wait until
          // the post digest queue is empty before enabling animations. By having two
          // calls to $postDigest calls we can ensure that the flag is enabled at the
          // very end of the post digest queue. Since all of the animations in $animate
          // use $postDigest, it's important that the code below executes at the end.
          // This basically means that the page is fully downloaded and compiled before
          // any animations are triggered.
          $rootScope.$$postDigest(function() {
            $rootScope.$$postDigest(function() {
              rootAnimateState.running = false;
            });
          });
        }
      );

Hope that helps. I'm looking forward to this PR getting merged. Keep up the great work.

@marcysutton
Copy link
Contributor

@ajoslin I'm still seeing aria-checked="true" on the second input.

@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 9, 2014

@marcysutton We use the same logic for checkbox ng-true-value and ng-false-value that angular does.

It sounds like ngAria should take ng-false-value and ng-true-value into account.

@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 9, 2014

@mzbyszynski I just ended up doing this in the link function:

      // no transition on initial load
      $$rAF(function() {
        element.addClass('transition');
      });

@marcysutton
Copy link
Contributor

@ajoslin gotcha! I'll file a bug.

@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 10, 2014

Fixed via 8bc9461

@ajoslin ajoslin closed this Dec 10, 2014
@ajoslin ajoslin removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 10, 2014
@ajoslin ajoslin deleted the wip-switch branch January 8, 2015 16:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants