-
Notifications
You must be signed in to change notification settings - Fork 900
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
Replaced dynatree with a modified version of bootstrap-treeview #10767
Conversation
ea7719d
to
dd1df57
Compare
@martinpovolny we had a discussion with @ZitaNemeckova about the coordination of this and her work with TreeBuilders. I think the remaining 2 tasks should be solved in a separate PR. Both of them are mass find-and-replace tasks and would cause a lot of merge conflicts in Zita's PRs. |
@martinpovolny @h-kataria @epwinchell @himdel ready for review ✨ |
So the plan is to merge this while carrying on reviewing and merging @ZitaNemeckova 's TreeBuilder PRs and then do the mass changes in a separate PR. |
@@ -463,6 +314,7 @@ function miqOnCheckCUFilters(tree_name, key, checked) { | |||
return true; | |||
} | |||
|
|||
// TODO: review this |
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.
TODO?
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.
sorry, forgot to remove ;)
@skateman : this PR in a single commit and having it rebased and then review the review feedback etc..... That illustrates it should not be done this way. |
aa9de7a
to
a65cd1a
Compare
@martinpovolny tests are now green |
@skateman Clicking on a VM in the right side list properly opens the tree and selects that node: Clicking on the root node shows the proper items on the right, but for some reason, resets the tree to only show the first level nodes below the root: |
@skateman the tree node images are a bit smaller with the new trees, can that be adjusted? |
@skateman in automate explorer, clicking on a tree node or on the same item in the list on the right fails to expand the tree node to show the child nodes in the tree, tho the right cell is updated properly. May happen elsewhere, but I have only seen it in automate tree: |
@skateman clicking on nodes in the Optimize / Utilization or Bottlenecks trees, they do not expand unless they had been expanded before. They used to. Another point to note here is that when first entering the Optimize / Utilization screen, no node was selected. In this branch, the root node is already selected. |
@skateman on compare screen (screenshot is from Hosts), even tho a subset of a tree node's children are checked, a full check is shown on the parent initially. This corrects itself if child nodes are checked/unchecked: |
@skateman I just noticed this:
I'm not sure how exactly to select the proper version, would need clearer instructions. Hopefully my above findings are not skewed, but let me know and I can retest. |
@dclarizio sorry, I forgot to tell that the new PF is released and now there is no need for the
The icon size is defined in patternfly by the patternfly guys, so I guess @serenamarie125 or @priley86 can tell if they can be enlarged.
It's a known issue and this PR fixes it.
This is a bug in bootstrap-treeview, the fix is on the way.
This is caused by a lazy-loading bug in bootstrap-treeview, I created a fix for that.
The same lazy-loading bug ...
In the current master with a clean session, the root node gets selected as in my branch. |
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
a65cd1a
to
e3e0b73
Compare
@dclarizio: do you think that this has enough quality so that we ca merge it and then track the remaining issues? |
e3e0b73
to
973a1dd
Compare
Checked commit skateman@973a1dd with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/controllers/application_controller/tree_support.rb
app/controllers/miq_policy_controller/miq_actions.rb
app/controllers/ops_controller/rbac_tree.rb
app/controllers/ops_controller/settings/cap_and_u.rb
app/controllers/report_controller/menus.rb
app/helpers/automate_tree_helper.rb
app/presenters/tree_builder.rb
app/presenters/tree_builder_sections.rb
app/presenters/tree_node_builder.rb
spec/models/automate_import_json_serializer_spec.rb
spec/presenters/tree_builder_roles_by_server_spec.rb
spec/presenters/tree_builder_servers_by_role_spec.rb
|
@martinpovolny @dclarizio @skateman LGTM Any minor styling tweaks we can address later. |
Merging this as most other issues are forthcoming from other PRs (mostly PF).
Bump ^^^ can either of you let us know if we can alter the tree node icon sizes as they can appear quite small depending on screen pixel density and they were already small prior to switching to these new trees? Thx, Dan |
✨ ✨ 🎉 🎉 🎉 🍷 🍰 🏆 |
Icons look too small @dclarizio, I agree. I'm talking with one of our Visual Designers now ... I'll get back to you soon. Checking with PF as well. It seems like the node expansion icons are incorrect, the font color is incorrect, and I'm still unclear if the icon size is what is being suggested by PatternFly or not ;). Stay tuned @skateman |
Used to be used before ManageIQ/manageiq#10767, dead since then.
locals_for_render
hasheslayouts/shared/tree
and TreeBuilderonDblClick
andonHover
events (see Dynatree features that maybe aren't necessary #10431)ldap_ous_tree
dynatree-title
andcfme-opacity-node
CSS classesThese tasks will be done in a separate PR:
miqDynatree
tomiqTree
in JS function names and filenames Rename functions, methods, files and routes containing the word dynatree #11050