-
Notifications
You must be signed in to change notification settings - Fork 28
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
Task 572: Configuration Page (settings and kick off windup) #278
Task 572: Configuration Page (settings and kick off windup) #278
Conversation
@klinki - I am requesting your review as I ran into some issues with the chosen-multiple component. Can you take a look and see if my changes here are reasonable? The "select all", "select none", and all selected states are still a WIP at the moment. |
@jsight It seems fine. |
d427343
to
b087af0
Compare
@klinki - Ok, functionality is ready to review now. |
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.
Just small detail and then it is good to merge.
Please keep both buttons visible all the time - select all / select none. As we have it for Custom Ruleset selection which is below.
It is little bit confusing button is changing.
@Input() | ||
set selectedApps(apps:RegisteredApplication[]) { | ||
this._selectedApps = apps; | ||
this.selectedAppsChange.emit(this._selectedApps); |
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'm surprised Angular is not complaining about this one. It is changing the value in @input. Angular usually triggers some ugly exceptions like
item modified after it has been checked
or something like that... Maybe it is intelligent enough not to complain, when you emit the same value as was the input one :)
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'm not totally sure that I understand what you mean. It isn't changing anything other than the property being @input() is it?
equalsCallback = (a1: RegisteredApplication, a2: RegisteredApplication) => a1.id === a2.id; | ||
|
||
selectAll() { | ||
this.selectedApps = this.availableApps; |
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.
You got me, I wanted to say you forgot to emit
, but you use set
property so it actually gets called... :)
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.
Yeah, it is relying on the setter property. :)
This changes the process of configuring Windup to allow for easier selection of applications.
… "select none" buttons.
e4a264d
to
c887e85
Compare
This changes the process of configuring Windup to allow for easier selection of applications.