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

Do not show containers tree in navlist #1129

Closed
wants to merge 1 commit into from

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Apr 24, 2017

Description

Currently the containers explorer page (https:///container/explorer) crashes when there is a large number of containers (e.g. ~4000).

The tree navigation list is not scalable, it has no pagination. This PR simply remove the containers-tree, because we do not have a better solution.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1443743

Screenshots
Bug
screenshot-localhost 3000-2017-04-24-15-48-00

Fix with filters closed:
screenshot-localhost 3000-2017-04-26-14-14-57

Fix with filters open:
screenshot-localhost 3000-2017-04-26-12-03-22

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@miq-bot add_label compute/containers, bug

If someone has a real system with 4000+ containers, please check ...

@simon3z @zeari @dkorn @moolitayer @himdel please review

@zeari
Copy link

zeari commented Apr 24, 2017

@isimluk any #1077 type magic we can do here?

@himdel
Copy link
Contributor

himdel commented Apr 24, 2017

The tree navigation list is not scalable, it has no pagination. This PR simply remove the containers-tree, because we do not have a better solution.

We had the same problem with a large number of VMs in the Compute > Infra > VMs section (and others) .. and ended up solving it by adding a user option to hide VMs from these trees (so only the sections remain).

So I guess a similar solution should be used here as well...


Look for hide_vms in code (and x_node_right_cell vs x_node), most of the change was introduced by @ZitaNemeckova in ManageIQ/manageiq#11387 - but be careful, since this introduced a few subtle bugs that had to be fixed after (ManageIQ/manageiq#12152, ManageIQ/manageiq#12614, #488, #671, ManageIQ/manageiq#12738, ManageIQ/manageiq#12504 and possibly others).

@ZitaNemeckova
Copy link
Contributor

@yaacov If you have any question about solution #1129 (comment) let me know.

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

@simon3z @himdel @serenamarie125

  • can we just remove the tree ?
  • do we need to use the solution used for vms ?

a. Use case - in common use cases we do not use this tree.
b. Screen real estate - this tree use half the screen ( and we usually don't use it ) maybe we can remove this left-bar completely ?
c. This BZ is 5.8, we can not do a big change here.

@himdel
Copy link
Contributor

himdel commented Apr 24, 2017

^ @dclarizio WDYT?

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

^ @nimrodshn FYI

@nimrodshn
Copy link
Contributor

nimrodshn commented Apr 24, 2017

+1 for removing the left bar altogether.

@himdel
Copy link
Contributor

himdel commented Apr 24, 2017

My main concern here is that yet again, this pushes Containers further away from the rest of manageiq, not closer. And that's something we need to handle sooner rather than later, because at some point , this becomes a problem, there's not much point in having manageiq as a unified interface if it's really not.

So.. backing away and leaving the higher-ups to decide ;)

@dclarizio
Copy link

@yaacov @simon3z Can we verify that the tree in the first accordion is of no value? From what I can see, it shows containers by pod. If that is usually a 1 to 1 relationship, then not showing the container leaf nodes probably isn't going to help much here.

If we DO remove that first accordion, then this should be changed to the non-explorer type list view screen, where the left side still shows the filters, as this will certainly be of value if you have thousands or more containers. No sense having an explorer view if there is no need to show the tree.

Can you verify with your PM as I'm sure the tree in the first accordion was seen as an initial requirement?

Thx, Dan

@Loicavenel
Copy link

I don't see the value of the tree if you have 1 to 1 relationship... then global filter is the best option... We should provide some default filter then...

@yaacov
Copy link
Member Author

yaacov commented Apr 24, 2017

I don't see the value of the tree if you have 1 to 1 relationship.

It looks like we remove the tree ...
@simon3z @serenamarie125 I need your opinion too, before continuing ...

We should provide some default filter then...

@simon3z I do not know what is a good default filter here :-( do we have an example somewhere for a similar case ?

@Loicavenel
Copy link

@yaacov you can have no filter for now and we will add it some in the future... at least customer can add its own

@serenamarie125
Copy link

@yaacov IMO, I'd remove the tree completely.

@dclarizio As far as deviating from ManageIQ, from a UX perspective, I think we want to move away from using a tree to filter regardless :) Due to the size of the UI, we are never going to be able to keep everything consistent. In the future, UX should provide a solution for the explorer tree/advanced filtering that MIQ can work towards gradually. I understand concerns about consistency, but I think that issues raised with the design in this situation are more important to address than to be consistent with an older design artifact.

@dclarizio
Copy link

@Loicavenel @serenamarie125 So, if we totally remove the left side, then there is no place to display any search filters that that user creates and saves. I think we need this for now until we do move forward with new UX.

My suggestion was (since we don't need the first accordion with the tree showing pods) was to change this back to a non-explorer list view (similar to Compute/Infrastructure/Hosts screen) which still has a left side panel, but no tree there, just a list of OOTB filters and user saved filters.

@yaacov We normally have default OOTB filters, such as "Environment / Prod", "Environment / Test", etc as well as maybe "Platform / RHEV" or "Status / Running" and "Status / Stopped". Whatever might be useful for Containters.

@yaacov yaacov force-pushed the do-not-show-containers-tree branch from 4e70636 to 95d4b2a Compare April 26, 2017 08:04
@yaacov yaacov force-pushed the do-not-show-containers-tree branch from 95d4b2a to 4bc0415 Compare April 26, 2017 09:15
@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2017

Checked commit yaacov@4bc0415 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

@yaacov yaacov changed the title [WIP] Do not show containers tree in navlist Do not show containers tree in navlist Apr 26, 2017
@miq-bot miq-bot removed the wip label Apr 26, 2017
@yaacov
Copy link
Member Author

yaacov commented Apr 26, 2017

@dclarizio @himdel please re-review

a. Removed the containers tree
b. Added default filters in:
ManageIQ/manageiq#14893

@himdel
Copy link
Contributor

himdel commented Apr 26, 2017

@yaacov can you also update the screenshots please?

@yaacov
Copy link
Member Author

yaacov commented Apr 26, 2017

can you also update the screenshots please?

yes !

the "Fix" image in the description include the filters from ManageIQ/manageiq#14893

screenshot-localhost 3000-2017-04-26-12-03-22
screenshot-localhost 3000-2017-04-26-12-04-22

@@ -199,17 +194,13 @@ def set_right_cell_vars(action)

# Replace the right cell of the explorer
def replace_right_cell(options = {})
action, replace_trees = options.values_at(:action, :replace_trees)
action, _replace_trees = options.values_at(:action, :replace_trees)
Copy link
Contributor

Choose a reason for hiding this comment

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

action = options[:action]?

@himdel
Copy link
Contributor

himdel commented Apr 26, 2017

@yaacov I'm not seeing any signs of this being a listnav screen. I still see a tree on the left, and no listnav-related changes in the code...

@himdel
Copy link
Contributor

himdel commented Apr 26, 2017

For reference, a listnav screen can be found in Hosts for example (http://localhost:3000/host/show_list) and looks like this..

listnav

EDIT:
There's also http://manageiq.org/docs/guides/ui/listnav ;)

@yaacov
Copy link
Member Author

yaacov commented Apr 26, 2017

There's also http://manageiq.org/docs/guides/ui/listnav ;)

Thanks, I am a little lost here, less lost now :-)

@yaacov
Copy link
Member Author

yaacov commented Apr 30, 2017

Closing, changing the view to use navlist, is different then just removing the tree ...
Continue discussion: in #1204

@yaacov yaacov closed this Apr 30, 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.

10 participants