-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lazy spinner directive #24
Conversation
|
||
angular | ||
.module('crowbarWidgets') | ||
.directive('suseLazySpinner', lazySpinner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep consistency, I'd use the same name for both directive's and function's name
Looks good so far. Please consider Bootstrap's helper classes and let me know to check again |
bd35b64
to
807442e
Compare
angular | ||
.module('crowbarWidgets') | ||
.directive('suseLazySpinner', suseLazySpinner) | ||
.constant('SUSELS_DEFAULT_DELAY', 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
.constant('SUSE_LAZY_SPINNER', {DEFAULT_DELAY: 2000});
?
I'm not fully convinced a directive file is the right place for a constant to be defined, but I don't have a strong suggestion neither
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this too. Changed now.
As for a separate constants
file, I think we can keep it inside the directive file as long as it's only one constant and it's used only by this directive. It can be refactored when the situation changes.
- added cleanup to suse-lazy-spinner directive to avoid leaks - added explicit spinner delay to landing page
51c7801
to
40111aa
Compare
40111aa
to
f09875f
Compare
|
||
function compileDirective(tpl) { | ||
var elem = angular.element(tpl); | ||
var compiledElem = $compile(elem)(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid Abbreviations.
Also, you could use
var elem = angular.element(tpl),
compiledElem = $compile(elem)(scope);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this one from some example. Changed.
Good work with testing. Please, review my comments and I'll check it again |
Looks good. Approved |
This directive adds a spinner which can be activated by a boolean value but will show only when active for more than specified
delay
.To use just insert:
<suse-lazy-spinner delay="3000" active="vm.running">
into your button or any other element where the spinner is required.Default delay (if not specified) is
2000
(2s).If you need feedback when the spinner is actually visible, bind a scope variable to
visible
attribute.E.g.
<suse-lazy-spinner active="vm.running" visible="vm.spinnerVisible">
.This can be used to simplify the styling as you can change styles on the parent elements in sync with the spinner events.
In this PR there is an example of using this directive on the landing page. Note that the test backend needs to be slowed down in order to see the effect of this directive.