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 #21135 - Add tooltip for overridden flag icon #4938

Merged
merged 1 commit into from
Nov 10, 2017

Conversation

shiramax
Copy link
Contributor

image

@theforeman-bot
Copy link
Member

Issues: #21135

@@ -50,7 +50,7 @@
<% @puppetclass.class_params.includes(:environments, :environment_classes, :lookup_values).each do |key| %>
<li data-used-environments=<%= key.environments.map(&:to_s).to_json %> >
<a data-toggle="tab" id="pill_<%= key.to_param %>" href="#<%= key.to_param %>" title="<%= key %>">
<div class="clip"><%= icon_text((key.override ? "flag": ""), key.to_s.tr('_',' '), :kind => 'fa') %></div>
<div class="clip"><%= icon_text((key.override ? "flag": ""), key.to_s.tr('_',' '), :kind => 'fa', :title=>"Overridden") %></div>
Copy link
Member

Choose a reason for hiding this comment

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

please make sure you extract the string, thanks :)

@shiramax
Copy link
Contributor Author

@ohadlevy ,
Done, thanks.

@ohadlevy
Copy link
Member

@orrabin can you review please?

@orrabin
Copy link
Member

orrabin commented Oct 31, 2017

lgtm. I think we should change the tooltip on the key to only appear for long keys, that would make it easier to find the flag tooltip but that could be addressed in a separate pr.

@ohadlevy
Copy link
Member

ohadlevy commented Nov 1, 2017

in that case, can you add a test please?

@shiramax
Copy link
Contributor Author

shiramax commented Nov 1, 2017

@orrabin,
I created the issue: http://projects.theforeman.org/issues/21543 thanks.
@ohadlevy ,
Which test do you want to add?

@ohadlevy
Copy link
Member

ohadlevy commented Nov 2, 2017

@shiramax something like the fact that the string is getting shorter if its longer than...?

@shiramax
Copy link
Contributor Author

shiramax commented Nov 2, 2017

@ohadlevy ,
Thanks, I think it should be addressed in a different PR,
I already created this PR: #4971 .

Copy link
Member

@dLobatog dLobatog left a comment

Choose a reason for hiding this comment

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

Thanks @shiramax , I think @ohadlevy 's comment is already addressed in #4971 , but the test isn't there.

Can you update this PR to include a test for that? It doesn't have to be an integration test, you can simply extract the key.size > 43 to a method in app/helpers and unit test the method.

click_link 'Smart Class Parameter'
assert_empty page.find("#pill_#{smart_class_parameter_short.id}-#{smart_class_parameter_short.key}")['data-original-title']
click_link 'Smart Variables'
assert_empty page.find("#pill_#{smart_variable_short.id}-#{smart_variable_short.key}")['data-original-title']

Choose a reason for hiding this comment

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

Put one space between the method name and the first argument.

smart_variable_short = FactoryBot.create(:variable_lookup_key, :key_type => '', :puppetclass => puppet_class_short, :key => "c"*40)
visit edit_puppetclass_path(puppet_class_short)
click_link 'Smart Class Parameter'
assert_empty page.find("#pill_#{smart_class_parameter_short.id}-#{smart_class_parameter_short.key}")['data-original-title']

Choose a reason for hiding this comment

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

Put one space between the method name and the first argument.

@shiramax
Copy link
Contributor Author

shiramax commented Nov 8, 2017

@dLobatog , @ohadlevy ,
I added the tests, Thanks.

@dLobatog
Copy link
Member

Thanks @shiramax !

@dLobatog dLobatog merged commit dbf496e into theforeman:develop Nov 10, 2017
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.

6 participants