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

Introduce option to separately configure storage and display ranges #72

Merged
merged 10 commits into from
Apr 13, 2020

Conversation

stklcode
Copy link
Contributor

@stklcode stklcode commented Mar 7, 2018

This PR instroduces a new option days_show to separately define the range of data collection and display in the dashboard widget.

days_show

Defaults to the same value on installation, but allows to keep long-term data in the database (e.g. to display in some custom evaluation) while keeping the dashboard widget clean and only show recent values.

@patrickrobrecht
Copy link
Member

patrickrobrecht commented Mar 8, 2018

Good work already!

For me it's a little confusing that the setting "Entries in top lists only for today" overrides "Period of data display in Dashboard" for the top lists if it's active. I suggest two separate settings "Period for chart" and "Period for top lists" (and removing "Entries in top lists only for today").

@stklcode
Copy link
Contributor Author

stklcode commented Mar 8, 2018

At the stage the "today" switch enables today only, otherwise chart period = top list period.
We should relabel the "for data display in Dashboard" to something like "for Dashboard chart" to clarify.

Possible alternative would be 3 periods without the today-checkbox. Might be over-configurability though.

Another question related to that:
Should we add a more sophisticated update method to restore previous behavior?

@patrickrobrecht
Copy link
Member

I don't think that a more sophisticated update method is necessary.

@websupporter
Copy link
Contributor

Why would someone want to separate those two values? Because

  1. He wants to store more data and
  2. doesn't want the dashboard to be flooded with data points

Why does he want to store more data than he shows? Because another tool is in use, which gives more insight. e.g. the extended plugin. This is, why I would opt for a filter instead of a UI to separate these values. If you only run the statify plugin, there is no benefit in separating those values. So to me, separating these two concerns is third party, like a CSV exporter plugin or the extended statify plugin or whatever.

To have the UI without a use case will most likely produce a lot of questions like "How can i see this data?" So in my eyes statify should have just one field, which defines the range in the dashboard and the save range, but the save range could be overwritten by third party via filter.

@stklcode
Copy link
Contributor Author

stklcode commented Mar 10, 2018

See the point. Argument against might be collecting data and occasionally having a look into it without a separate plugin. Maybe not the average Joe's behavior though and you could still add a filter by hand.

So something like this (with some sanitization), which allows modification of all 3 parameters, might do the job without adding new attributes, 95% of the users probably never missed... Could prepare another PR for that.

// Initialize default dashboard configuration.
$cfg = array(
	'days'  => (int) self::$_options['days'],
	'limit' => (int) self::$_options['limit'],
	'today' => ( 0 !== (int) self::$_options['today'] ),
);

// Apply filter to allow modification of dashboard config.
$cfg = apply_filters( 'statify__dashboard_config', $cfg );

(basically 1 additional line so far, just merged the 3 existing variables into an array)

@Zodiac1978
Copy link
Member

This is, why I would opt for a filter instead of a UI to separate these values. If you only run the statify plugin, there is no benefit in separating those values.

There is a use case. People could want to check today, yesterday, last week, last month, last year values (or similar), like in some of the other widgets seen in #119

Copy link
Member

@patrickrobrecht patrickrobrecht left a comment

Choose a reason for hiding this comment

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

In my tests, the option "period of data display in the dashboard" (days_show) is not saved - it is 14 days - even after the bugfix for the wrong file path.

inc/class-statify-dashboard.php Outdated Show resolved Hide resolved
@patrickrobrecht patrickrobrecht merged commit 2ab4a90 into master Apr 13, 2020
@patrickrobrecht patrickrobrecht deleted the dashboard-filter branch April 13, 2020 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants