Skip to content
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

Fixes #10738 - Added javascript to focus on alert in puppetclasses tab #2445

Closed
wants to merge 1 commit into from

Conversation

ShimShtein
Copy link
Member

No description provided.

@@ -0,0 +1,6 @@
$(document).on('ContentLoad', function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

New JS files also need to be listed in config/environments/production.rb for precompilation unless only required by another.

@ShimShtein
Copy link
Member Author

@domcleal, Added the file to the config, and to the hostgroups form.

@domcleal
Copy link
Contributor

Sorry, just looked again and we already have class_edit.js which is used for Puppet class related things on host/host group forms, so this should be part of that file rather than a new one.

Or perhaps it can be part of the errorFields code in onContentLoad() in the main application.js, in case it finds a use elsewhere?

@ShimShtein
Copy link
Member Author

@domcleal Thanks for pointing me to the class_edit.js. You are right, these two need merging. Actually I have overlooked it, because it doesn't conform to the host_edit_* syntax that I very liked in the interfaces. I prefer to rename the class_edit to host_edit_puppetclasses, and add my functions to it.

About moving it to application.js - it's very specific to the UI layout of the puppetclasses, so i don't want to pollute the general file. I will try to extract a general/framework part out of it as a part of aspect separation effort that I do now. For now I would like to keep it separate.

@domcleal
Copy link
Contributor

Actually I have overlooked it, because it doesn't conform to the host_edit_* syntax that I very liked in the interfaces. I prefer to rename the class_edit to host_edit_puppetclasses

Probably because it's also used in config_groups, it's a bit more generic than just host (or host + hostgroup) editing.

About moving it to application.js - it's very specific to the UI layout of the puppetclasses, so i don't want to pollute the general file. I will try to extract a general/framework part out of it as a part of aspect separation effort that I do now. For now I would like to keep it separate.

Okay, sounds good. I hadn't checked how common that style of error reporting was.

@ShimShtein
Copy link
Member Author

it's a bit more generic than just host (or host + hostgroup) editing.

In this case, there is justification for separation, I can't assume that both host and config_groups will use tabbed layout to show puppet classes.

Okay, sounds good. I hadn't checked how common that style of error reporting was.

Basically every tab needs to report that it has validation errors. Now we handle a bit more specific case of a classic field validation errors. We need to add some kind of support for errors that are not specific to a certain field.

@@ -1,6 +1,6 @@
<% if obj.errors[:puppetclasses].any? %>
<%= alert :class => "alert-block alert-danger base in fade",
Copy link
Member

Choose a reason for hiding this comment

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

if you add has-error to the alert class and remove .form-group from app/assets/javascript/application.js:62, the focus will work using the generic error focus handling.

@ShimShtein
Copy link
Member Author

Thanks @tbrisker, looks more elegant than my suggestion.

@domcleal
Copy link
Contributor

Merged as fa229d3, thanks @ShimShtein.

@domcleal domcleal closed this Jun 16, 2015
bkearney added a commit to bkearney/foreman that referenced this pull request Feb 10, 2016
- Added an AIX icon using lzaps dummy languages. Used a green similar to teh AIX logo
- Renamed Freebsd to the text format which is used by the operating system page
dLobatog pushed a commit to dLobatog/foreman that referenced this pull request Feb 11, 2016
- Added an AIX icon using lzaps dummy languages. Used a green similar to teh AIX logo
- Renamed Freebsd to the text format which is used by the operating system page
dLobatog pushed a commit to dLobatog/foreman that referenced this pull request Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants