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

Make containers dashboard a shareable view and fix wrong pf-utilization-trend-chart directive #7296

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

zeari
Copy link

@zeari zeari commented Mar 15, 2016

change pf-utilization-chart to pf-utilization-trend-chart in containers dashboard.
there was a name change in the new PF version.
Before:
before_pf-donut_rename

After(notice the donut is now aligned to the left):
after_pf_donut_rename

@miq-bot add_label bug, ui, providers/containers

Please review @himdel @abonas
cc @simon3z

@abonas
Copy link
Member

abonas commented Mar 15, 2016

don't you need to change it here as well?

@zeari zeari force-pushed the fix_dashboard_donut_trends branch from a85e6f6 to ca4adc1 Compare March 15, 2016 14:53
@zeari
Copy link
Author

zeari commented Mar 15, 2016

don't you need to change it here as well?

Good catch!

@abonas
Copy link
Member

abonas commented Mar 15, 2016

you might want to do a shareable file with common code and include it in both files, PRs such as this are quite an example why it would be a good idea to do it. less chance to forget, less need for copy paste.
see an example here #6985

@himdel
Copy link
Contributor

himdel commented Mar 15, 2016

Would be great to have the common code unified, but as for this PR, looks good :)

(Also, seems like pf-card, pf-select, pf-trends-chart and pf-aggregate-status-card all survived the 2->3 change, so hopefully we should get no more angular-patternfly surprises :) (I haven't checked patternfly itself though.))

@zeari zeari force-pushed the fix_dashboard_donut_trends branch from ca4adc1 to d764ad5 Compare March 17, 2016 13:00
@zeari
Copy link
Author

zeari commented Mar 17, 2016

Would be great to have the common code unified...
you might want to do a shareable file with common code and...

@himdel @abonas Moved dashboard haml to shared partial.

@abonas
Copy link
Member

abonas commented Mar 17, 2016

pls change the title to reflect what this PR does (2 changes now).
other than that - great!

@zeari zeari changed the title change pf-utilization-chart to pf-utilization-trend-chart in containe… Make containers dashboard a shareable view and fix wrong pf-utilization-trend-chart directive Mar 17, 2016
@@ -0,0 +1,125 @@
.container-fluid.container-tiles-pf.containers-dashboard.miq-dashboard-view{"ng-app" => "containerDashboard",
Copy link
Contributor

Choose a reason for hiding this comment

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

.containers-dashboard in show.html.haml but .single-provider-containers-dashboard in _show_dashboard.html.haml

Copy link
Author

Choose a reason for hiding this comment

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

.containers-dashboard in show.html.haml but .single-provider-containers-dashboard in _show_dashboard.html.haml

Right. Thats hard to fix without #6981. We will know what to do when we will have those images\icons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's right.. can you (or @abonas ?) reply to Eric there? I'm almost sure we do want font icons but maybe I'm wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way, an alternative would be to make the shared partial only from line 9 on ..but yeah, that has its own problems :)

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that's right.. can you (or @abonas ?) reply to Eric there? I'm almost sure we do want font icons but maybe I'm wrong?

I dont think thats up to us :\

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure @epwinchell was asking about the actual practical limitations..

dclarizio pushed a commit that referenced this pull request Mar 17, 2016
Make containers dashboard a shareable view and fix wrong pf-utilization-trend-chart directive
@dclarizio dclarizio merged commit c253de1 into ManageIQ:master Mar 17, 2016
@dclarizio dclarizio added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 17, 2016
@dclarizio
Copy link

@zeari please see @himdel comments above. I missed them (screen didn't refresh), so please address them in a follow up PR. Thx, Dan

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.

5 participants