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

Jump to bottom

only show_limit_control on Acquisition/Overview #18017

Merged
merged 10 commits into from Sep 17, 2021
Merged

only show_limit_control on Acquisition/Overview #18017

merged 10 commits into from Sep 17, 2021

Conversation

geekdenz
Copy link
Contributor

Description:

Not super beautiful fix, but it works.

Review

@geekdenz geekdenz marked this pull request as draft September 16, 2021 04:25
@tsteur
Copy link
Member

tsteur commented Sep 16, 2021

@geekdenz did you have a look at @sgiehl comment in #17987 (comment) ? Did changing this one to true maybe not fix the problem?

@geekdenz
Copy link
Contributor Author

@tsteur I already checked his comment and fixed it here:
24a4cfe
Did you see that or is there something wrong with the fix?

@geekdenz
Copy link
Contributor Author

build js

@geekdenz
Copy link
Contributor Author

@sgiehl I believe this means it is fixed:

https://builds-artifacts.matomo.org/matomo-org/matomo/m-17965/49074/Comparison_row_evolution.png

sgiehl
sgiehl requested changes Sep 16, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@geekdenz please don't build JS files in pull request that actually didn't change anything in the piwik.js. This was actually forgotten when merging another PR and should either be directly fixed on 4.x-dev or with a separate PR. Here it will lead to merge conflicts.

The solution you proposed might fix the problem described in the issue, but actually might not be the simplest / best way.
The default value of show_limit_control is true. So it is enabled by default. See

matomo/core/ViewDataTable/Config.php

Lines 329 to 335 in be72457

/**
* Controls whether the limit dropdown (which allows users to change the number of data shown)
* is always shown or not.
*
* Normally shown only if pagination is enabled.
*/
public $show_limit_control = true;

or specifically for the evolution charts here:

matomo/plugins/CoreVisualizations/Visualizations/JqplotGraph/Evolution/Config.php

Lines 41 to 45 in 1155273

$period = Common::getRequestVar('period');
if ($period !== 'range') {
$this->show_limit_control = true;
$this->show_periods = true;
}

So the reason why the limit control is not shown for the referrer evolution chart is, that it's disabled in the according report class:

matomo/plugins/Referrers/Reports/GetReferrerType.php

Line 105 in 1155273

$view->config->show_limit_control = false;

Simply removing this one line should fix the issue. Overwriting that again doesn't make sense at all.

Tim-Hinnerk Heuer and others added 2 commits September 16, 2021 21:12
@geekdenz
Copy link
Contributor Author

Sorry, @sgiehl and thanks for the suggestion.

@geekdenz geekdenz marked this pull request as ready for review September 17, 2021 02:04
@geekdenz geekdenz added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Sep 17, 2021
@tsteur tsteur merged commit d2dd657 into 4.x-dev Sep 17, 2021
@tsteur tsteur deleted the m-17965 branch September 17, 2021 03:52
@tsteur
Copy link
Member

tsteur commented Sep 17, 2021

I'll close #17965

fyi @geekdenz if you put fix #17965 in the PR description then it will close the related issue automatically when merging.

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

@sgiehl sgiehl sgiehl requested changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@geekdenz @tsteur @sgiehl