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

Fix comparison trends might be displayed incorrect for certain languages #18832

Merged
merged 8 commits into from Mar 8, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 21, 2022

Description:

fixes #18802

Review

@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 21, 2022
@sgiehl sgiehl force-pushed the fixcomparisontrend branch 3 times, most recently from 05140f1 to 2a66056 Compare February 21, 2022 15:43
@sgiehl sgiehl marked this pull request as ready for review February 21, 2022 16:16
@sgiehl sgiehl added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Feb 21, 2022
@sgiehl sgiehl marked this pull request as draft February 22, 2022 16:41
@sgiehl sgiehl marked this pull request as ready for review February 23, 2022 15:39
@sgiehl sgiehl added the Needs Review PRs that need a code review label Feb 23, 2022
@sgiehl sgiehl requested a review from tsteur February 23, 2022 15:40
@sgiehl
Copy link
Member Author

sgiehl commented Feb 23, 2022

@tsteur I have simplified the PR. The additional columns will now only be requested by the sparkline visualization and thus won't occur in any API output.

@tsteur tsteur added this to the 4.9.0 milestone Feb 23, 2022
@tsteur
Copy link
Member

tsteur commented Feb 23, 2022

I've put it for now into 4.9. I had a quick look and it looks OK but I can't do a full review due to time. Be great for someone to do later a full review

Copy link
Contributor

@peterhashair peterhashair left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Mar 5, 2022
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 7, 2022
@sgiehl
Copy link
Member Author

sgiehl commented Mar 7, 2022

@diosmosis It seems the test I had added to this PR starts failing after #18761 has been merged in. The sparklines lost their color. See https://builds-artifacts.matomo.org/matomo-org/matomo/fixcomparisontrend/54586/Comparison_visits_overview_widget_sv.png

I guess the javascript method to initialize the sparklines is called before the colormanager, which is in Vue, has been loaded fully. The sparline requests contain "lineColor":[null,null]
Would you mind having a look and maybe pushing a fix on this branch?

@diosmosis
Copy link
Member

@sgiehl that's weird, ColorManager is still in vanilla JS. I'll take a look.

@diosmosis
Copy link
Member

@sgiehl should be fixed

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Mar 8, 2022
@sgiehl sgiehl merged commit a6a7cf7 into 4.x-dev Mar 8, 2022
@sgiehl sgiehl deleted the fixcomparisontrend branch March 8, 2022 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Swedish translation when displaying negative or zero values
4 participants