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 #5140 - User children taxonomies did not show tooltip 'Parent is already selected' #1374

Closed
wants to merge 1 commit into from

Conversation

@isratrade
Copy link
Member

isratrade commented Apr 13, 2014

No description provided.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Apr 13, 2014

@elobato, the users form is now using the loc_org_tabs partial which doesn't gray out the child taxonomies, but rather shows a tooltip 'Parent is already selected'. This is true for all 'taxable taxonomies' (i.e. users, domains, smart proxies, hostgroupsts, etc). If I remember correctly, we discussed the behavior and to add a tooltip rather than disabling child taxonomies and moving them to the selected column. We can can discuss this behavior again. I think it had to do with not submitting the id's of the child taxonomies and/or wanting to so that ensure certain child taxonomies remain selected if the parent taxonomy is removed.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Apr 14, 2014

fyi, original issue #5140 is "children taxonomies are not greyed out"

@dLobatog

This comment has been minimized.

Copy link
Member

dLobatog commented Apr 19, 2014

I think it's okay to leave the tooltip rather than blocking.

I see only one problem with this PR when I tested it, it only takes into account the status of the multi select when the page is loaded.

If a parent taxonomy is moved to the left (unselected), it'll still show up the tooltip for children taxonomies.

👍 to merge after that. It's still a bit confusing but that's good enough I think.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Apr 22, 2014

@elobato, I discussed this with @abenari and we feel that the tool-tips are not so beneficial after a user starts deselecting items. To keep the tool-tips in sync, it can't be work client-side with jquery and needs to make an ajax call which we think is overkill just for toolips. For example, if there are multiple parents A and A/B that are selected and A/B/C is not selected. When you de-select A/B, I can't automatically remove the tool-tip of it's descendants (C), since it's also a descendant of A. I need to make an ajax call to refresh
'data-descendants' => obj.children_of_selected_organization_ids.to_json
Instead, I just remove the tooltip 'Parent is already selected' after an item is deselected. It's a "hack" but I think the cost/benefit ratio justifies it. What do others think?

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented May 1, 2014

@thomasmckay, @ehelms, Jenkins is showing a failure on katello only, but it didn't show the failed test. Any ideas?

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented May 1, 2014

@isratrade it's since been fixed

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented May 1, 2014

[test]

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented May 21, 2014

@elobato, any changes needed here?

@dLobatog

This comment has been minimized.

Copy link
Member

dLobatog commented Jun 13, 2014

@isratrade I tried recreating what you described, but the tooltip does not get removed when any item is deselected. See below:

…s already selected'
@@ -1,3 +1,4 @@
<%= javascript 'users' %>

This comment has been minimized.

Copy link
@isratrade

isratrade Jun 25, 2014

Author Member

@elobato, this is why the PR didn't work on the users form, but it works on the others. There was a silent javascript error that it didn't find taxonomy_added

@dLobatog

This comment has been minimized.

Copy link
Member

dLobatog commented Jun 26, 2014

Works well, I think the point for having #1543 exists so that it can be merged for 1.5.x because this one will get in for 1.6, correct me if I'm wrong @domcleal ?

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented Jun 26, 2014

@elobato yup

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jun 29, 2014

I agree. Let's merge PR #1543 so it can be cherry-picked for 1.5.x

@dLobatog

This comment has been minimized.

Copy link
Member

dLobatog commented Jul 22, 2014

Now that #1543 is in for 1.5.x, this goes in for 1.6.

Merged as 18d7939 , thanks @isratrade

@dLobatog dLobatog closed this Jul 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.