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

Container project dashboard view #1527

Merged
merged 2 commits into from
Aug 29, 2017
Merged

Conversation

zeari
Copy link

@zeari zeari commented Jun 12, 2017

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1461618
Dashboard view for container projects
with data:
screencapture-localhost-3000-container_project-show-12-1503408359344

with no data available:
screencapture-localhost-3000-container_project-show-35-1503325881909

@serenamarie125 @Loicavenel @simon3z

correct pod statistics depend on: ManageIQ/manageiq#10470

@zeari
Copy link
Author

zeari commented Jun 12, 2017

@miq-bot add_label compute/containers, wip

@Loicavenel
Copy link

@zeari Thanks for sharing. There are other things we should look at:

  • Some Performance Charts: CPU and Memory graph over last 30 days
  • List of all Pods and their status: I can image a table with 2 columns Pod Name and Status.

@serenamarie125 We need a better presentation here especially if we add the suggestion above

@zeari zeari force-pushed the projects_dashboard branch 2 times, most recently from c516519 to 7a47fdb Compare June 19, 2017 10:22
@simon3z
Copy link
Contributor

simon3z commented Jul 7, 2017

cc @dclarizio

@zeari
Copy link
Author

zeari commented Aug 17, 2017

@Loicavenel @simon3z @serenamarie125, This is the design im currently going with:
[old screenshot]

Some Performance Charts: CPU and Memory graph over last 30 days
List of all Pods and their status: I can image a table with 2 columns Pod Name and Status.

this generally covers your request as well as show quotas. Can anyone softly ack this design so i can proceed?

@serenamarie125
Copy link

@zeari yes definitely in the right direction!

Here is the layout I initially provided:
projectleveldash

Aggregate status cards should definitely be on the top.
The layout of the trend card which i used is a bit different from PatternFly, and we will be contributing it to Ang-PF this month or early September. I would try using "Card with Single Trend" from http://www.patternfly.org/angular-patternfly/#/api/patternfly.card.component:pfCard%20-%20Trends ... and we will then be adding a mechanism to have the text on the right.

Hope that helps!

@Loicavenel
Copy link

@zeari I really like it.. this will look great..

@zeari zeari force-pushed the projects_dashboard branch 3 times, most recently from 15ffbb2 to 3bcd089 Compare August 21, 2017 14:38
@Loicavenel
Copy link

@zeari can you clarify the Limits is CPU while graph is Cores... We see CPU limits to 2 but 4 Cores.. so, CPU are multiple-cores?

@zeari
Copy link
Author

zeari commented Aug 22, 2017

@zeari can you clarify the Limits is CPU while graph is Cores... We see CPU limits to 2 but 4 Cores.. so, CPU are multiple-cores?

I put in some random metrics instead of waiting for them to generate. Ill make sure this doesnt happen normally.

@zeari zeari force-pushed the projects_dashboard branch 2 times, most recently from 8758222 to f157c00 Compare August 22, 2017 11:05
@zeari
Copy link
Author

zeari commented Aug 22, 2017

@yaacov @zakiva Please review.

@zeari zeari force-pushed the projects_dashboard branch 2 times, most recently from eea20d7 to 56852ee Compare August 22, 2017 11:15
@@ -19,7 +19,7 @@
expect(response.body).to_not be_empty
expect(assigns(:breadcrumbs)).to eq([{:name => "Projects",
:url => "/container_project/show_list?page=&refresh=y"},
{:name => "Test Project (Summary)",
{:name => "Test Project (Dashboard)",
Copy link
Author

Choose a reason for hiding this comment

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

Since dashboard is the default view

@zeari
Copy link
Author

zeari commented Aug 22, 2017

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Container project dashboard view Container project dashboard view Aug 22, 2017
@miq-bot miq-bot removed the wip label Aug 22, 2017
@dclarizio dclarizio self-assigned this Aug 22, 2017
@dclarizio
Copy link

@h-kataria can you help @zeari make sure this is following the new direction we have for the PF dashboards? Give some direction if needed.

@zeari I haven't looked at the codeclimate issues, but 80 is quite high. Can you please take a look to see if that can be knocked down?

Thx, Dan

@zeari
Copy link
Author

zeari commented Aug 22, 2017

@zeari I haven't looked at the codeclimate issues, but 80 is quite high. Can you please take a look to see if that can be knocked down?

mostly code duplication. Ill try to throw what i can into a mixin.

@serenamarie125
Copy link

@zeari this looks great! Thanks for updating to match the mocks !

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM!

@serenamarie125
Copy link

@miq-bot add_label ux/approved

@simon3z
Copy link
Contributor

simon3z commented Aug 23, 2017

LGTM 👍
cc @dclarizio

@dclarizio dclarizio requested a review from himdel August 23, 2017 13:48
@dclarizio
Copy link

@zeari I haven't looked at the codeclimate issues, but 80 is quite high. Can you please take a look to see if that can be knocked down?

mostly code duplication. Ill try to throw what i can into a mixin.

I see a bunch of syntax checks as well . . . @himdel can you recommend which cc issues we can overlook and which should be taken care of?

Copy link
Contributor

@h-kataria h-kataria left a comment

Choose a reason for hiding this comment

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

Generally this looks good, but @zeari we will need to convert these into Angular and to consume API endpoints(wherever possible) once we have #1901 completed/merged.

@h-kataria
Copy link
Contributor

@zeari if you can knock down some of the code climate warnings that will be good.

@himdel
Copy link
Contributor

himdel commented Aug 25, 2017

@zeari Look at https://codeclimate.com/github/ManageIQ/manageiq-ui-classic/pull/1527 please.

Looks like that service is just a copy of the original one, with very little changes, please try to add a mixin to avoid copy-pasting :)

EDIT: oh, Harpreet already mentioned it :)

@zeari zeari force-pushed the projects_dashboard branch 2 times, most recently from 1fff023 to a8dfa11 Compare August 28, 2017 10:33
@miq-bot
Copy link
Member

miq-bot commented Aug 28, 2017

Checked commits zeari/manageiq-ui-classic@41e6a99~...feffdde with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 1 offense detected

spec/controllers/container_project_controller_spec.rb

@zeari
Copy link
Author

zeari commented Aug 28, 2017

Looks like that service is just a copy of the original one, with very little changes, please try to add a mixin to avoid copy-pasting :)

Moved to ContainerServiceMixin.

@himdel
Copy link
Contributor

himdel commented Aug 29, 2017

@zeari Sorry, but the same goes for the JS bits.. container_dashboard_controller.js and container_project_dashboard_controller.js are exactly the same code,

^ this is a problem, but it's one I don't want to block this on since we already have the exact same thing for ems_infra_dashboard_controller.js.

@h-kataria @AparnaKarve @zeari We can't afford to have multiple dashboard controllers, each doing the exactly same thing, on a bit different set of entities. This is topology all over again (where the count is now 6 copies, not 3).

Will one of you take care of merging those 3 dashboard controllers back into one or should I take it?

@himdel
Copy link
Contributor

himdel commented Aug 29, 2017

Tested in the UI, both the container dashboard and the project dashboard seem to work.

@himdel himdel merged commit 6d9a40a into ManageIQ:master Aug 29, 2017
@chessbyte chessbyte added this to the Sprint 68 Ending Sep 4, 2017 milestone Oct 16, 2017
@h-kataria
Copy link
Contributor

@zeari now that #1901 is merged, i think you can go-ahead and convert Container Project dashboard to follow the same pattern. Feel free to ping me if you have questions

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.

9 participants