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 repeating values on Y-axis of C&U charts #40

Merged
merged 4 commits into from
Jan 24, 2017
Merged

Fix repeating values on Y-axis of C&U charts #40

merged 4 commits into from
Jan 24, 2017

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Jan 2, 2017 •
edited
Loading

Fix repeating values on Y-axis of C&U charts. This is caused by not having labels with appropriate precision.

This issue has two parts. First when chart is loaded, should adjust precision to values. Most of the times it already is done by ruby, but not always. Second, adjust precision when you click on legend, hide/show series of data, change dynamically labels precision according to shown data.

Links [Optional]

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

Screenshots

Before:
screencast from 2017-01-24 11-35-26
After:
screencast from 2017-01-24 11-37-15

Steps for Testing/QA

(converted from ManageIQ/manageiq#13289)

@PanSpagetka PanSpagetka mentioned this pull request Jan 2, 2017
Merged
@mzazrivec mzazrivec added the wip label Jan 2, 2017
@chessbyte chessbyte added the bug label Jan 5, 2017
@chessbyte chessbyte changed the title [WIP]Fix repeating values on Y-axis of C&U charts [WIP] Fix repeating values on Y-axis of C&U charts Jan 5, 2017
@PanSpagetka PanSpagetka force-pushed the miq_pr_13289 branch 12 times, most recently from 66ac68f to e74bdb3 Compare January 10, 2017 15:04
@PanSpagetka PanSpagetka changed the title [WIP] Fix repeating values on Y-axis of C&U charts Fix repeating values on Y-axis of C&U charts Jan 10, 2017
@PanSpagetka
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Jan 10, 2017
@himdel himdel self-assigned this Jan 13, 2017
@martinpovolny
Copy link
Member

there are some style issues (codeclimate)

and why don't you place the code to miq_c3.js?

@PanSpagetka PanSpagetka force-pushed the miq_pr_13289 branch 2 times, most recently from 6a0f5a0 to 2a07611 Compare January 17, 2017 12:56
himdel
himdel reviewed Jan 19, 2017
View reviewed changes
app/assets/javascripts/miq_application.js
var format = data.axis.y.tick.format;
max = _.max(getChartColumnDataValues(data.data.columns));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a var too. (CC may warn about it, but ignore that, double var is way less bad then forgetting about it and having an accidental global)

himdel
himdel reviewed Jan 19, 2017
View reviewed changes
app/assets/javascripts/miq_application.js
var min_showed = getChartFormatedValue(format, min);
var change_format = true;
// if there are no valid values or there is only single values big enough, then not change formating function
// TODO MOVE IT TO FUNCTION
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a TODO :)

himdel
himdel reviewed Jan 19, 2017
View reviewed changes
app/assets/javascripts/miq_c3.js
var format = ManageIQ.charts.chartData.candu[this.config.bindto.split('_').pop()].xml.miq.format;
var minMax = getMinMaxFromChart(this);
// if there are no valid values or there is only single values big enough, then return
if (!minMax || minMax[0] === minMax[1]) {
Copy link
Contributor

@himdel himdel Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

identical code introduced in miq_application.js

@PanSpagetka PanSpagetka force-pushed the miq_pr_13289 branch 4 times, most recently from 114a6fb to dfb7669 Compare January 20, 2017 13:36
@himdel
Copy link
Contributor

himdel commented Jan 23, 2017

LGTM now, except this could really use a before & after screenshot, and one more thing...

The changeFormat bit looks rather close to the recalculatePrecision bit, so maybe these could be a shared function..

@PanSpagetka
Fix repeating values on Y-axis of C&U charts
510075f 
https://bugzilla.redhat.com/show_bug.cgi?id=1383821

(cherry picked from commit 8c1585103969cd55270d6b98889a1d49b54f9760)
@PanSpagetka
Add dynamic change of Y axis
d3218be 
(cherry picked from commit 5bc7efab2573a508c58f7e32ee5275a5caf646e1)
@PanSpagetka
Move shared code to function
8e83bc1
@PanSpagetka
Copy link
Contributor Author

@himdel Code moved to function, added screenshots :)

@miq-bot
Copy link
Member

miq-bot commented Jan 24, 2017

Checked commits PanSpagetka/manageiq-ui-classic@510075f~...c63af43 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

@himdel
Copy link
Contributor

himdel commented Jan 24, 2017

Tested in the UI, it wooorks! :)

@himdel himdel merged commit 9cb01d3 into ManageIQ:master Jan 24, 2017
@himdel himdel added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 24, 2017
@simaishi
Copy link
Contributor

@PanSpagetka @himdel I assume this should be euwe/yes?

@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit 8a5f4d5217e9ea09512febd6e596fabc6560ccc6
Author: Martin Hradil <[email protected]>
Date:   Tue Jan 24 13:35:25 2017 +0000

    Merge pull request #40 from PanSpagetka/miq_pr_13289
    
    Fix repeating values on Y-axis of C&U charts
    (cherry picked from commit 9cb01d3f54e697d57c7267c5268797b57c7a3c68)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1416093

@himdel
Copy link
Contributor

himdel commented Jan 25, 2017

@simaishi yes, thanks for adding it! :)

@PanSpagetka PanSpagetka mentioned this pull request Feb 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel left review comments

Assignees

@himdel himdel

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

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