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.

Sign up for GitHub

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

Jump to bottom

Fix units formating for grouped charts #382

Merged
merged 2 commits into from
Feb 24, 2017
Merged

Fix units formating for grouped charts #382

merged 2 commits into from
Feb 24, 2017

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Feb 14, 2017 •
edited
Loading

Following PR for #335. Formatting function was not added to charts because of name of columns.
Specs depend on ManageIQ/manageiq#13998

Screenshots

Before:
screencapture-localhost-3000-host-show-10000000000017-1487063016635

After:
screencapture-localhost-3000-host-show-10000000000017-1487072812137

Links

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

romanblanco
romanblanco reviewed Feb 14, 2017
View reviewed changes
lib/report_formatter/c3.rb Outdated
@@ -91,7 +91,8 @@ def build_document_header
unless mri.graph[:type] == 'Donut' || mri.graph[:type] == 'Pie'
mri.chart[:legend] = {:position => 'bottom'}
end
format, options = javascript_format(mri.graph[:columns][0], nil)
column = mri.performance&.dig(:group_by_category) ? mri.graph[:columns][0].split(/_+/)[0..-2].join('_') : mri.graph[:columns][0]
Copy link
Member

@romanblanco romanblanco Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Don't use &. yet, for the same reason as here: ManageIQ/manageiq#10663

column =  mri.performance.try(:dig, :group_by_category) ? 
  mri.graph[:columns][0].split(/_+/)[0..-2].join('_') :
  mri.graph[:columns][0]

Copy link
Member

@martinpovolny martinpovolny Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

don't use dig for the same reason

Copy link
Member

Choose a reason for hiding this comment

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

use fetch_path instead of dig

@martinpovolny
Copy link
Member

Maybe a simple test? I believe I have added a way to test the charts in the past so it should be possible to add a test for this particular feature.

@skateman
Copy link
Member

skateman commented Feb 18, 2017 •
edited
Loading

A small thing: my physics teacher used to say that if you have a chart X/Y vales with units, then you don't need the same unit in parentheses on the axis' label, or in this case in the chart's title.

@PanSpagetka PanSpagetka force-pushed the fix-units-in-CU-grouped branch from acc4079 to 69dfff4 Compare February 20, 2017 14:08
mzazrivec
mzazrivec reviewed Feb 21, 2017
View reviewed changes
lib/report_formatter/c3.rb Outdated
@@ -91,7 +91,10 @@ def build_document_header
unless mri.graph[:type] == 'Donut' || mri.graph[:type] == 'Pie'
mri.chart[:legend] = {:position => 'bottom'}
end
format, options = javascript_format(mri.graph[:columns][0], nil)

binding.pry
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:(

This was referenced Feb 21, 2017
Merged
Merged
@martinpovolny
Copy link
Member

The specs patch could use a less generic comment ;-)

@mzazrivec mzazrivec modified the milestone: Sprint 55 Ending Feb 27, 2017 Feb 24, 2017
@mzazrivec
Copy link
Contributor

@PanSpagetka Please fix the rubocop warnings.

@PanSpagetka PanSpagetka force-pushed the fix-units-in-CU-grouped branch from 8219974 to 02b98dd Compare February 24, 2017 10:34
@PanSpagetka PanSpagetka force-pushed the fix-units-in-CU-grouped branch from 02b98dd to d1b648b Compare February 24, 2017 10:46
@miq-bot
Copy link
Member

miq-bot commented Feb 24, 2017

Checked commits PanSpagetka/manageiq-ui-classic@2ccc5a6~...d1b648b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 1 offense detected

lib/report_formatter/c3.rb

@mzazrivec mzazrivec self-assigned this Feb 24, 2017
@mzazrivec mzazrivec added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 24, 2017
@mzazrivec mzazrivec merged commit 0ba48fa into ManageIQ:master Feb 24, 2017
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2017

Euwe backport (to manageiq repo) details:

$ git log -1
commit 787ebd50f2b7b70574da20a106806e709ca0b842
Author: Milan Zázrivec <[email protected]>
Date:   Fri Feb 24 12:44:57 2017 +0100

    Merge pull request #382 from PanSpagetka/fix-units-in-CU-grouped
    
    Fix units formating for grouped charts
    (cherry picked from commit 0ba48fa69ce24fe35287385bab1149b8b3de072c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1413105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzazrivec mzazrivec mzazrivec left review comments

@romanblanco romanblanco romanblanco left review comments

@skateman skateman skateman left review comments

@martinpovolny martinpovolny martinpovolny left review comments

Assignees

@mzazrivec mzazrivec

Labels
bug charts euwe/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@PanSpagetka @martinpovolny @skateman @mzazrivec @miq-bot @simaishi @romanblanco @chessbyte