Fixes #9453 - improve tooltips on puppetclass selection forms #2607
Conversation
|
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
|
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
theforeman-bot
added
Needs testing
Waiting on contributor
and removed
Waiting on contributor
labels
| links.attr('onclick', 'remove_puppet_class(this)'); | ||
| links.attr('data-original-title', __('Click to undo adding this class')); | ||
| links.attr('data-original-title', __('Click to remove ')+_.escape(fullname)); |
There was a problem hiding this comment.
Use sprintf, not concatenation.
|
Please use commas between ticket numbers in the commit message. |
|
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
theforeman-bot
added
Needs re-review
Needs testing
Waiting on contributor
and removed
Waiting on contributor
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@domcleal addressing your comments led to a bit of refactoring to DRY and remove dead code. Please review, [test] failure is random katello failure. |
| options = options_for_puppetclass_selection(klass, type) | ||
| text = remove_link_to_function(truncate(klass.name, :length => 28), options) | ||
| content_tag(:span, text).html_safe + | ||
| remove_link_to_function('', options.merge!(:class => 'glyphicon glyphicon-minus-sign')) |
There was a problem hiding this comment.
Use merge and not ! in all of these calls, as the result is being passed into the method.
|
re ticket #2879, it looks like this is already working correctly in develop. I think there was a change to the CSS of tooltips somewhere else lately, might that have done it? I can't see what in your PR changes this.
Ditto with #5733, isn't that the same issue? This most directly seems to be #9453, though you've implemented something different. I do prefer leaving the class name in the tooltip though, as I think it's more helpful when it's truncated. |
|
@domcleal It seems like #2879 and #5733 are indeed already fixed in develop, although there are some issues with the tooltips as they are currently. I seem to recall a certain case where they were broken but can't seem to reproduce now. As far as I'm concerned they can both be marked as resolved and I'll edit the commit message for this to only #9453 (or also add another new issue for the other changes in this if you wish). |
|
Yep, please just use #9453, you've improved the wording. If you can mark the other two with release 1.10.0 and resolved, then that's good. Thanks. |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
tbrisker
changed the title
|
@domcleal updated commit message, addressed comments and marked other bugs as resolved. |
|
@tbrisker I think on the original bug they want it to say "Add this class" and "Remove this class", as it's a bit more 'friendly'. I'd say just change that & I'll merge. Works fine. |
|
I think it's an improvement as it is, just ignore that suggestion? |
|
One reason it's important to keep the class name in the tooltip is because the names in the actual list may be truncated, as there's limited space. The tooltips allow users to see the long names, which is what the other tickets in the title were originally about (but are currently fixed in develop). |
…s in host/hostgroup/configgroup edit