-
Notifications
You must be signed in to change notification settings - Fork 356
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
Conversation
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 |
@yaacov If you have any question about solution #1129 (comment) let me know. |
@simon3z @himdel @serenamarie125
a. Use case - in common use cases we do not use this tree. |
^ @dclarizio WDYT? |
^ @nimrodshn FYI |
+1 for removing the left bar altogether. |
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 ;) |
@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 |
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... |
It looks like we remove the tree ...
@simon3z I do not know what is a good default filter here :-( do we have an example somewhere for a similar case ? |
@yaacov you can have no filter for now and we will add it some in the future... at least customer can add its own |
@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. |
@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. |
4e70636
to
95d4b2a
Compare
95d4b2a
to
4bc0415
Compare
Checked commit yaacov@4bc0415 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@dclarizio @himdel please re-review a. Removed the containers tree |
@yaacov can you also update the screenshots please? |
yes ! the "Fix" image in the description include the filters from ManageIQ/manageiq#14893 |
@@ -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) |
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.
action = options[:action]
?
@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... |
For reference, a listnav screen can be found in Hosts for example (http://localhost:3000/host/show_list) and looks like this.. EDIT: |
Thanks, I am a little lost here, less lost now :-) |
Closing, changing the view to use navlist, is different then just removing the tree ... |
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
![screenshot-localhost 3000-2017-04-24-15-48-00](https://cloud.githubusercontent.com/assets/2181522/25337787/da951d42-2905-11e7-905e-8ef0fb26cae4.png)
Bug
Fix with filters closed:
![screenshot-localhost 3000-2017-04-26-14-14-57](https://cloud.githubusercontent.com/assets/2181522/25431821/d9933ca4-2a8a-11e7-9896-e68a08e964cd.png)
Fix with filters open:
![screenshot-localhost 3000-2017-04-26-12-03-22](https://cloud.githubusercontent.com/assets/2181522/25428843/b938f454-2a7f-11e7-9f7c-92f0e8ed68f9.png)