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 popularity graph start at y=0 #445

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

daniel-beck
Copy link
Contributor

@daniel-beck daniel-beck commented Oct 30, 2020

Amends #402 to always start the graph at y=0. Showing a narrower range is misleading at a glance. Even if it shows small differences more clearly, the downside doesn't seem worth it.

FYI @ARUNMOHANRAJ471

Example: Showing rapidly changing popularity

While both https://plugins.jenkins.io/trilead-api/ and https://plugins.jenkins.io/okhttp-api/ gained popularity, the relative gains are very different, so should result in different graphs.

Before

trilead-api
okhttp-api

After

trilead-api
okhttp-api

Example: Showing fairly unchanging popularity

Additionally, this will now not over-emphasize fairly insignificant differences in reported installation counts.

Before

matrix-auth
git-parameter

After

matrix-auth
git-parameter

@daniel-beck daniel-beck requested a review from a team as a code owner October 30, 2020 21:15
@@ -13,12 +13,11 @@ const calculateHeight = (total) => {
};

const calculateMinMax = (data) => {
const minValue = Math.min(...data);
const maxValue = Math.max(...data);
// calculate a dynamic value to center the graph
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're no longer centering things here, so I'm not sure how well this comment applies anymore.

@daniel-beck
Copy link
Contributor Author

The "worst case" for the range algorithm is when the max install count is just over a power of 10. Then the top of the graph is 2x that power of 10, like for TestComplete:

after

The current behavior isn't great either though (IMO), I would even argue there's an improvement:

before

@zbynek zbynek merged commit bed22ee into jenkins-infra:master Oct 30, 2020
@zbynek
Copy link
Contributor

zbynek commented Oct 30, 2020

This is what I was suggesting in the original PR but I thought I was in the minority on that. Thanks for the PR!

@daniel-beck
Copy link
Contributor Author

@zbynek Thanks, I appreciate the agreement and merge – although it might be better to give potentially controversial PRs like this a little more time to collect feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants