Make containers view navlist #1204

yaacov · 2017-04-30T13:10:00Z

Description

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

This PR fixes that problem by using the navlist layout.

BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1443743
https://bugzilla.redhat.com/show_bug.cgi?id=1446247 ( fixed by changing the way tag edit works )

Screenshots

yaacov · 2017-04-30T13:13:36Z

@miq-bot add_label compute/containers, bug

cc// @simon3z @zeari @moolitayer @himdel

yaacov · 2017-04-30T15:05:29Z

~~@himdel Help, can't find where to enable "filters" in the left bar, any clue ? :-)~~

[EDIT]
Fixed :-)

yaacov · 2017-04-30T19:16:42Z

@simon3z @zeari @moolitayer @himdel please review

yaacov · 2017-05-01T05:29:57Z

cc @nimrodshn

zeari · 2017-05-01T08:56:31Z

app/views/container/show.html.haml

+- elsif @showtype == "main"
+  = render :partial => "layouts/textual_groups_generic"
+- elsif @showtype == "topology"
+  = render :file => 'container_topology/show'


I dont think topology is relevant here.

@himdel it seems this part is mostly the same for a lot of entities. any smart refactoring we can do for this?

I dont think topology is relevant here.

@zeari , Right 👍 thanks fixed.

Loicavenel · 2017-05-01T12:53:38Z

I really like this approach, make it easier to use

miq-bot · 2017-05-03T08:43:26Z

Checked commits yaacov/manageiq-ui-classic@bfae3bb~...6b579f7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 1 offense detected

app/views/configuration/_ui_2.html.haml

⚠️ - Line 42 - Line is too long. [413/160]

yaacov · 2017-05-03T08:43:30Z

@himdel Thanks 👍 done

p.s.
In https://github.com/ManageIQ/manageiq-ui-classic/pull/1204/files#diff-8d54b753701485016bbb2ce8bf826529R1 we have a big if that look suspicious but I was afraid to delete ... do we need it ?

himdel · 2017-05-03T09:14:34Z

@yaacov 👍

About that big if .. have you tried removing it to see what breaks? :) But .. you probably do, at least, it looks rather similar to other app/views/*/show.html.haml..

yaacov · 2017-05-03T09:17:18Z

you probably do

leaving it :-)

nimrodshn · 2017-05-03T12:57:49Z

I really like the way it looks now make much more sense to me 👍 .

nimrodshn · 2017-05-03T18:48:48Z

@yaacov @himdel I know that with other entities where there are realtionships shown in the navbar we keep a count in parentheses - does it make sense for us to do the same here? i.e. instead of having State \ Running it will show State \ Running (num of containers running) .

yaacov · 2017-05-03T18:53:01Z

with other entities where there are realtionships week keep a count in parentheses

@nimrodshn I looked and didn't find that, I may be looking in the wrong place :-) , can you be more specific, what entity and where the data (screenshot may help here)

yaacov · 2017-05-03T18:59:24Z

with other entities where there are realtionships week keep a count in parentheses

@nimrodshn
Ahhh ... realtionships ... not filters, it's implicitly 1, a container has 1 ems, 1 project, 1 ... , so we do not explicitly display (1) it's clear from the context.

nimrodshn · 2017-05-03T19:04:05Z

Yeah wasn't sure if it applies here - Still, looks really good @yaacov!

yaacov · 2017-05-07T10:31:53Z

@himdel @martinpovolny please re-review.

martinpovolny · 2017-05-09T14:47:35Z

Sorry, was not at work.

The code looks good. But I lack the time to test this in the UI today and tomorrow at least. Can you get someone from your group run this in the UI and test it in detail? It's a big change and proper UI testing is needed.

Thx!

martinpovolny · 2017-05-09T14:48:20Z

Ping @nimrodshn, @zeari ? ^^^

yaacov · 2017-05-09T14:59:01Z

cc @cben ^^

dkorn

The code looks good. But I lack the time to test this in the UI today and tomorrow at least. Can you get someone from your group run this in the UI and test it in detail? It's a big change and proper UI testing is needed.

@martinpovolny I tested the change locally, works for me 👍
LGTM

simon3z · 2017-05-10T10:21:51Z

@yaacov @moolitayer should this have the label for backporting?

yaacov · 2017-05-10T10:29:22Z

should this have the label for backporting?

@miq-bot add_label fine/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1443743
Target is 5.8.1

p.s.
The filters in the screenshots are from:
ManageIQ/manageiq#14893

cc// @simon3z @moolitayer

cben · 2017-05-10T11:03:01Z

Works for me 👍 . links (relationships) seem to work. breadcrumbs seem to work.

I don't see the filters without ManageIQ/manageiq#14893, upside is I confirm this works without it:

cben · 2017-05-10T13:35:32Z

Code reviewed (superficially), massive deletion Looks Great To Me ✂️ 🔥 🎆

codeclimate go home, you're drunk.

dclarizio · 2017-05-10T13:40:43Z

@martinpovolny looks like @cben took this for a spin, perhaps ready to go?

martinpovolny · 2017-05-10T19:30:16Z

Thanks for the testing!

Make containers view navlist (cherry picked from commit 70aec6d) https://bugzilla.redhat.com/show_bug.cgi?id=1458934 https://bugzilla.redhat.com/show_bug.cgi?id=1458935

simaishi · 2017-06-08T14:54:33Z

Fine backport details:

$ git log -1
commit 10be54bc8b9ec852e7162b7c9a4c17d62a62e3b8
Author: Martin Povolny <[email protected]>
Date:   Wed May 10 21:29:44 2017 +0200

    Merge pull request #1204 from yaacov/make-containers-navlist
    
    Make containers view navlist
    (cherry picked from commit 70aec6d45b200a44f7c81bb7357cb916c7290aff)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1458934
    https://bugzilla.redhat.com/show_bug.cgi?id=1458935

yaacov mentioned this pull request Apr 30, 2017

Do not show containers tree in navlist #1129

Closed

miq-bot added wip bug compute/containers labels Apr 30, 2017

yaacov force-pushed the make-containers-navlist branch 7 times, most recently from 50af654 to ec65a47 Compare April 30, 2017 14:42

yaacov force-pushed the make-containers-navlist branch 7 times, most recently from 8bb6de6 to 16185d5 Compare April 30, 2017 18:59

yaacov changed the title ~~[WIP]Make containers view navlist~~ Make containers view navlist Apr 30, 2017

miq-bot removed the wip label Apr 30, 2017

yaacov mentioned this pull request Apr 30, 2017

Add default filters for the container page ManageIQ/manageiq#14893

Merged

yaacov mentioned this pull request May 1, 2017

Update miq-shortcuts ManageIQ/manageiq#14951

Merged

zeari reviewed May 1, 2017

View reviewed changes

yaacov force-pushed the make-containers-navlist branch from 16185d5 to ca4959a Compare May 1, 2017 09:14

dclarizio assigned martinpovolny May 1, 2017

yaacov added 2 commits May 3, 2017 11:37

Remove the topology route (thanks zeai)

e770233

Remove the tree presentor (thanks himdel)

6b579f7

yaacov force-pushed the make-containers-navlist branch from ca4959a to 6b579f7 Compare May 3, 2017 08:40

dkorn approved these changes May 10, 2017

View reviewed changes

miq-bot added the fine/yes label May 10, 2017

martinpovolny merged commit 70aec6d into ManageIQ:master May 10, 2017

martinpovolny added this to the Sprint 61 Ending May 22, 2017 milestone May 10, 2017

simaishi added fine/backported and removed fine/yes labels Jun 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make containers view navlist #1204

Make containers view navlist #1204

yaacov commented Apr 30, 2017 •

edited

Loading

yaacov commented Apr 30, 2017

yaacov commented Apr 30, 2017 •

edited

Loading

yaacov commented Apr 30, 2017

yaacov commented May 1, 2017

zeari May 1, 2017

yaacov May 1, 2017

Loicavenel commented May 1, 2017

miq-bot commented May 3, 2017

yaacov commented May 3, 2017

himdel commented May 3, 2017

yaacov commented May 3, 2017

nimrodshn commented May 3, 2017

nimrodshn commented May 3, 2017 •

edited

Loading

yaacov commented May 3, 2017

yaacov commented May 3, 2017

nimrodshn commented May 3, 2017

yaacov commented May 7, 2017

martinpovolny commented May 9, 2017

martinpovolny commented May 9, 2017

yaacov commented May 9, 2017

dkorn left a comment

simon3z commented May 10, 2017

yaacov commented May 10, 2017

cben commented May 10, 2017

cben commented May 10, 2017 •

edited

Loading

dclarizio commented May 10, 2017

martinpovolny commented May 10, 2017

simaishi commented Jun 8, 2017

Make containers view navlist #1204

Make containers view navlist #1204

Conversation

yaacov commented Apr 30, 2017 • edited Loading

yaacov commented Apr 30, 2017

yaacov commented Apr 30, 2017 • edited Loading

yaacov commented Apr 30, 2017

yaacov commented May 1, 2017

zeari May 1, 2017

Choose a reason for hiding this comment

yaacov May 1, 2017

Choose a reason for hiding this comment

Loicavenel commented May 1, 2017

miq-bot commented May 3, 2017

yaacov commented May 3, 2017

himdel commented May 3, 2017

yaacov commented May 3, 2017

nimrodshn commented May 3, 2017

nimrodshn commented May 3, 2017 • edited Loading

yaacov commented May 3, 2017

yaacov commented May 3, 2017

nimrodshn commented May 3, 2017

yaacov commented May 7, 2017

martinpovolny commented May 9, 2017

martinpovolny commented May 9, 2017

yaacov commented May 9, 2017

dkorn left a comment

Choose a reason for hiding this comment

simon3z commented May 10, 2017

yaacov commented May 10, 2017

cben commented May 10, 2017

cben commented May 10, 2017 • edited Loading

dclarizio commented May 10, 2017

martinpovolny commented May 10, 2017

simaishi commented Jun 8, 2017

yaacov commented Apr 30, 2017 •

edited

Loading

yaacov commented Apr 30, 2017 •

edited

Loading

nimrodshn commented May 3, 2017 •

edited

Loading

cben commented May 10, 2017 •

edited

Loading