-
Notifications
You must be signed in to change notification settings - Fork 5
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
[FEATURE] Add content changes widget #69
[FEATURE] Add content changes widget #69
Conversation
Requires rebasing and change in new Fluid template file if #70 is merged first. |
Rebase coming up. |
Adds a new widget which shows: * Pending publishing state change, if any. Lists records which have starttime / endtime in the future. * Changed records along with name of user who made the change. Attempts to select a total of $limit (default 5) records while distributing the number of records equally between the "pending" and "changed" sets, giving preference to the "pending" set: * If limit is 5 and 3 "pending" were selected, only 2 "changed" will be selected. * If limit is 5 and no "pending" were selected, all 5 are selected from the "changed" set. If there are no records in a group the table containing the record data is not shown. if there are no results in either of the two groups, a "nothing to show" message is shown instead.
a3c2bba
to
acd7fc2
Compare
@haassie The CGL checker is not very helpful - it can't be run locally without "composer install" which I try to avoid for sideloaded extensions, and the Travis build does not report the exact error (it is in "fix" mode but the changes are not seen). Any chance you could make the Travis job more verbose about CGL? |
I added 308fa07 so we have a small diff snippet showing what is wrong and how to fix it. |
Do we want to check if the records shown are available for the specific user? Do they have the right permission etc? This would also be great in other widgets like the |
Yep that would be a good addition, but a little bit difficult to implement (at least for the optimised query in this widget). The problem is that the access check is a post-processing and might remove too many items / the query has to select far too many items and crop the data set in PHP. |
Maybe we can do it "incremental"? So start with x number of items, check if there are any items removed, check the next x until it reaches the number of items we would like to show? |
That is one possibility, but we might end up dispatching a lot of queries for unprivileged users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we at least remove the pencil when a page is not editable for the current user? Now it will end up in nasty errors.
@@ -9,6 +9,7 @@ | |||
$widgetRegistry->registerWidget('sysLogErrors', \FriendsOfTYPO3\Dashboard\Widgets\SysLogErrorsWidget::class); | |||
$widgetRegistry->registerWidget('t3News', \FriendsOfTYPO3\Dashboard\Widgets\T3NewsWidget::class); | |||
$widgetRegistry->registerWidget('documentation', \FriendsOfTYPO3\Dashboard\Widgets\DocumentationWidget::class); | |||
$widgetRegistry->registerWidget('contentChanges', \FriendsOfTYPO3\Dashboard\Widgets\ContentChangesWidget::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registration of widgets changed. Can you merge master into your branch and do some little refactoring in the registration?
foreach ($results as &$result) { | ||
$result['type'] = $table; | ||
$result['label'] = BackendUtility::getRecordTitle($table, $result); | ||
$result['icon'] = $iconFactory->getIconForRecord($table, $result, Icon::SIZE_SMALL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure if we want coloured icons in the dashboard. We want to keep the dashboard simple and no overdue of color. On the other hand it might be a recognisable icon for the user.
@irenesacchi what do you think?
@haassie Is there any way I could convince you to not drop the PHP registration API? It seems like a very annoying restriction - PHP allows you to do conditions etc.. exposes that you have references to your widget class file. I would absolutely keep things PHP-first. |
|
I already had a discussion with Claus and next days I will be thinking if we have made the right decision. Have to check some use-cases. |
TL;DR: YAML should be a secondary way to use public PHP API provided by EXT:dashboard. To name a few use cases:
It gets even more pronounced if we include dashboards:
But the whopper:
I completely understand the reasons for YAML when it is appropriate, but the problem is that in many cases it really isn't appropriate - especially if it completely disables the ability to add things in PHP. My choice would be to add YAML support as a secondary way to store widget/dashboard configurations or just provide defaults, but register the things contained in the YAML through public PHP API that is equally possible to use directly, thus skipping the need for a YAML file. Doing so also leaves you implementation agnostic and allow you to do things like take pageTSconfig as source of widget/dashboards configurations through simple adapters. |
The extension is now merged in core and we this repository won't be used anymore. The widget might still be an interesting one, so if you want to open up a forge ticket for it, or maybe even provide a patch, that would be awesome! |
Adds a new widget which shows:
which have starttime / endtime in the future.
the change.
Attempts to select a total of $limit (default 5) records
while distributing the number of records equally between
the "pending" and "changed" sets, giving preference to
the "pending" set:
will be selected.
selected from the "changed" set.
If there are no records in a group the table containing
the record data is not shown. if there are no results in
either of the two groups, a "nothing to show" message
is shown instead.