-
Notifications
You must be signed in to change notification settings - Fork 897
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
Angular host editor #3859
Angular host editor #3859
Conversation
70d904a
to
7042fe1
Compare
@AparnaKarve can you review/test this, i still need to add more Jasmine tests to this PR and some further refactoring if possible, but i need someone to review this. |
751d43a
to
09c1fc5
Compare
046d1a3
to
b8c88ad
Compare
b8c88ad
to
ffb1466
Compare
ffb1466
to
d399ff1
Compare
2. Used auth_credentials_angular haml for Default and Remote Tabs
- This commit handles any issues found during angular host editor testing. - Further cleanup of controller code and fixed existing spec tests. Added Jasmine tests for host editor.
Also made a few other adjustments in the haml
d399ff1
to
836e5eb
Compare
52f3854
to
62a7442
Compare
@dclarizio this PR is ready for review/testing. |
Checked commits h-kataria@aad7338 .. h-kataria@62a7442 with rubocop 0.32.1 and haml-lint 0.13.0 app/controllers/host_controller.rb
app/views/host/_form.html.haml
app/views/layouts/_auth_credentials_form.html.haml
app/views/layouts/angular-bootstrap/_auth_credentials_angular_bootstrap.html.haml
app/views/layouts/angular/_auth_credentials_angular.html.haml
app/views/layouts/angular/_form_buttons_verify_angular.html.haml
app/views/layouts/angular/_multi_auth_credentials.html.haml
spec/controllers/host_controller_spec.rb
|
62a7442
to
bf35a05
Compare
@dclarizio, @h-kataria and I worked together on this PR and have reviewed and tested each other's commits. |
@@ -10,7 +10,6 @@ | |||
//= require services/miq_db_backup_service | |||
//= require directives/scheduler/updateDropdownForFilter | |||
//= require directives/scheduler/updateDropdownForTimer | |||
//= require directives/miq_calendar |
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.
@h-kataria any reason for this change? You've broken the retirement editor ;).
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.
@himdel i don't recall why that was removed, maybe just an oversight or result of bad merge/rebase from master
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.
OK, just checking, Milan will add it back, thanks :).
content_tag(:li, | ||
:class => "#{options[:class]} #{active == id ? 'active' : ''}", | ||
:id => "#{id}_tab", | ||
'ng-click' => "changeAuthTab('#{id}');") do |
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.
@h-kataria Do you remember why this change was needed please?
Because now it is putting that ng-click
in every tab, everywhere ... which will break once those other screens are actually angular and try to call it.
I'll move it to options
, and I'm assuming this is only needed in app/views/layouts/_multi_auth_credentials_form.html.haml
but .. if you know more.. :)
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.
@himdel It looks like this change was added to keep track of the current tab that user is on in the form, it was added by @AparnaKarve in 5b227b5 maybe she remembers if there was some other purpose behind adding that ng-click
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.
@himdel Yeah, the main purpose was to handle tab-specific logic in the angular controller.
If there are tabs, there will be tab-specific logic, meaning we would always have changeAuthTab
in every angular controller that deals with tabs and hence chances of something breaking probably don't exist.
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.
Aah, thanks :) Ok, I'll move it to that template then, so that we explicitly pass {'ng-click' => "changeAuthTab('something')"}
in there...
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.
Fixed in ManageIQ/manageiq-ui-classic@2b1da75
No description provided.