Refactor Storm dashboard

Hi Magnity,

this task is already implemented in the storm_contrib modules. After publishing it will be tested by the community and if it is working it can be moved into the strom core.

Greetings
Carsten Müller

Hi,

just a hint. Storm Contrib is published as a dev version. We are still working on features and bug fixes. As you there is always not enough time. I would be happy if you could test and review it and then give us some feedback.

Thanks
Carsten

Hi Carsten, which of the Storm Contrib modules implements this? I'd like to have a look.

Hi juliangb,

the main function is in the module storm_extension, storm_extension.module, function storm_extension_dashboard()
It makes an module_inkvoke_all() so there is a hook for each module to submit links to the dashboard function. The most links are set int he function storm_extension_storm_dashboard_links(), but later the storm core item links can be moved to their modules.

Other modules can now submit their links to the dashboard. The informations for the links are passed in an array.

It ist not perfekt but it works to invoke the links of other modules too.

I just made some important changes, so you have to wait until the changes are submitted in a new build of the dev version

Greetings
Carsten

There was a bug in the storm_extension_dashboard() in the access check. It is fixed and with the next dev snapshot available

Hi Carsten, I came across this issue whilst looking to see what changes might need to be made for #1392484: Separate Storm Knowledgebase into separate project.

What is the status of this functionality in storm_contrib? It might be a good time to move it into Storm core so that we can remove some of the hardcoded bits of stormkb and other storm modules.

Hi Julian,

it is implemented in storm contrib and working. Each dashboard item can be selected/deselected so it is possible to set which items should be displayed.

For moving it into storm core, we have to move the main funtions to the storm.module, replace the hook and change the hook calls in all storm contrib moduls. It is a good improvement but it is some work.

I can write a patch (one for storm and one for storm contrib) if i have some time left.

Hi Carsten,

Did you get time at all to look at this?

Only to notice I'm currently working on it.

Following Patch replaces current dashboard with a adjustable dashboard.
In /admin/settings/storm/storm are options to change weight and disable each icon.
Links will be added through module_invoke from the other modules.
Also it adds a dashboard block, with own weight and active settings. It can be used to have some important links on every page.
An alter hook is also added, if some wished to change weight, path, names, ... of the links.
With the new hook_storm_dashboard_types it is possible to add additional own dashboard. weight, active settings and all hooks can the be used for this own dashboard. storm_dashboard_get_links(false, 'own dashboard') can then be used to get this links.
Storm Contrib uses this feature, to add a reports dashboard. (stormtimetracking_extension)

Patches are for storm, stormknowledgelab and storm contrib.

Okay. Didn't break anything else.
Created Issues for storm contrib (#1412596: Refactor Storm dashboard) and knowledge lab (#1412598: Refactor Storm dashboard)

One thing what came in my mind right now. Would be an update path, so that settings made in storm contrib would be used here.
Will do this right away.

Added update.

A few comments from my read through of the code - nothing major. Just about to test.

I also wondered whether if is worth having defaults for the link array. That way, if a module wanted to implement a link but leave some elements as default, it would be a lot simpler.

+++ b/storm.install
@@ -92,8 +92,31 @@ function storm_update_6201() {
+function storm_update_6202() {

Would you mind adding a doc block? (Helps drush users).

+++ b/storm.module
@@ -191,15 +203,196 @@ function storm_theme() {
+ * @internal param \when $bool $check_active

What does this line mean?

+++ b/storm.module
@@ -191,15 +203,196 @@ function storm_theme() {
+ * @param unknown_type $a
+ * @param unknown_type $b
+ * @return unknown_type

Not sure that this adds anything!

+++ b/storm.module
@@ -191,15 +203,196 @@ function storm_theme() {
+    $links[] = array(
+    'title' => t('Configuration'),
+    'icon' => 'stormconfiguration',
+    'path' => 'admin/settings/storm',
+    'params' => array(),
+    'access_arguments' => 'Storm: access administration pages',
+    'node_type' => '',
+    'add_type' => '',
+    'map' => array(),
+    'weight' => 15,
+    );
+    $links[] = array(
+      'theme' => 'storm_dashboard_link',
+      'title' => t('Attributes'),
+      'icon' => 'stormattributes',
+      'path' => 'storm/attributes',
+      'params' => array(),
+      'access_arguments' => 'Storm: access administration pages',
+      'map' => array(),
+      'weight' => 14,
+    );

Indentation is inconsistent.

+++ b/storm.module
@@ -191,15 +203,196 @@ function storm_theme() {
+  elseif ($type == 'block') {
+    // block uses same links as url, but have own settings
+    $links = module_invoke_all('storm_dashboard_links', 'page');
+  }

Surely this has the possibility of duplicates if a module implements the link separately for page and block?

Powered by Dreditor.

Just tested. I really like this.

Two additional comments:
1 - Could we add a min-height to the block in order that when it is used as a header the icons don't cut off?

2 -

+++ b/storm.theme.inc
@@ -89,38 +89,149 @@ function theme_storm_view_item($label, $value) {
+  $headers = array(t('Link'), t('active'), t('Weight'));

I think this would look better capitalised - 'Active'.

Powered by Dreditor.

Okay, changed all the suggestions. Thanks for the review!
Also added that the menu entry for each dashboard will be added automatically.

For the Links is only the title and path needed.

I hope you have maybe the time to look over this once again. I would like, if this would make it into the next release. If so i would do also a beta release of storm contrib, because this deletes one module, so i would like to do this before :)

One minor thing from my read through of the patch from #18. I'm just going to test.

+++ b/storm.module
@@ -191,15 +213,209 @@ function storm_theme() {
+      'description' => t('You can disable or reorder the links from the the !dashboard here', array('!dashboard' => l(t('dashboard'), 'storm'))),

"The" is written twice.

Powered by Dreditor.

I've just tested the latest patch. All looks good.

Just committing #18, omitting the extra 'the'.

@kfritsche, hope that's ok by you!

Thanks! I have no problem with this. If you didn't do this today, my plan was to do this tomorrow. :)