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

Invert change comparison when displaying percents in the UI … #15098

Merged
merged 6 commits into from Nov 18, 2019

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Nov 3, 2019

and add extra _change_from metric to support API calculation of from change percent when there are more than two periods.

Changes:

  • add invert_compare_change_compute query parameter that computes change to the selected period. this is done so clients don't have to do any strange work to get the change_from value from the other comparison series. useful for the mobile app, eg.
  • add _change_from metrics to the output. This allows users to get the new metrics that are displayed through the API when any number of periods are compared.

…extra _change_from metric to support API calculation of from change percent when there are more than two periods.
@diosmosis diosmosis added c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review labels Nov 3, 2019
@diosmosis diosmosis added this to the 3.13.0 milestone Nov 3, 2019
@diosmosis diosmosis removed the c: Design / UI For issues that impact Matomo's user interface or the design overall. label Nov 14, 2019
@tsteur
Copy link
Member

tsteur commented Nov 17, 2019

@diosmosis is this UI change expected? https://builds-artifacts.matomo.org/matomo-org/matomo/comparison-percents-ui/36774/Comparison_totals_tooltip.png

In https://builds-artifacts.matomo.org/matomo-org/matomo/comparison-percents-ui/36774/Comparison_visits_overview_widget.png

image

do we still expect the first row to show the minus or should maybe the second have the plus percentage? I guess we wouldn't want to mix the behaviour as it would be inconsistent. (not 100% sure why we're making this change that's why I'm asking I guess)

BTW there's also at least one system test failing.

Code looks good otherwise

@diosmosis
Copy link
Member Author

You're comparing 12-17 w/ 1-31, so it's showing the change from 1-31 => 12-17, ie, jan 12-17 is 42% lower when compared with jan 1-31, so it's correct.

@diosmosis diosmosis merged commit 21b49c1 into 3.x-dev Nov 18, 2019
@diosmosis diosmosis deleted the comparison-percents-ui branch November 18, 2019 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants