Add showFullscreen and showCompact for dashboard #356

NavidSassan · 2022-09-29T09:59:54Z

Re-implementation of #294.

cla-bot · 2022-09-29T09:59:56Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @NavidSassan

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

NavidSassan · 2022-09-29T10:08:51Z

Corporation CLA signed

bobapple · 2022-09-29T10:13:21Z

@cla-bot check

sukhwinder33445

Please use the mockup below to style the full screen view:

I have not yet reviewed BpDashboardFullscreenTile.php, because it will change when you add changes according to the mockup.

application/controllers/IndexController.php

sukhwinder33445 · 2023-07-24T09:04:14Z

library/Businessprocess/Web/Component/DashboardFullscreen.php

+    /** @var string */
+    protected $tag = 'div';
+
+    protected $defaultAttributes = array(


Please use short array syntax and fix align.

I copied that from the library/Businessprocess/Web/Component/Dashboard.php file. Should I change both files, just the DashboardFullscreen.php or leave it as it is?

Please only change in this class, as it is a new class. Leave the old code as it is.

library/Businessprocess/Web/Component/DashboardFullscreen.php

library/Businessprocess/Web/Controller.php

public/css/module.less

library/Businessprocess/Web/Component/DashboardFullscreen.php

library/Businessprocess/Web/Controller.php

slalomsk8er · 2023-07-31T20:48:26Z

@sukhwinder33445, is your request about the cosmetics and not about the content and information shown? Please confirm.

If not, I fear, that your request goes directly against our needs and why we sponsor this code!

Our requirement is to be able to see the process titles inside of the configs. In our use case, It's quite useless to know, on a big wall mounted sign, that X business critical processes inside a department, clinic, branch or team are kaput because the next question in mind is which one and there isn't a mouse or any other input available to find out without, then and there.

Please, have a look at the original pull request Add a nice dashboard for kiosk mode for a explanation and a mock-up.

sukhwinder33445 · 2023-08-01T09:33:27Z

Hi @slalomsk8er,
The request is also about the content and the information that should be displayed.

The main reason why we should not show the titles of the nodes in each configuration is that we would then have problems with scalability. A large environment can have multiple configurations with multiple (hundreds) of nodes. In this case, only a few configurations with their node titles and states can be displayed on the dashboard page.

In the following screenshot, you can see the current state of the full screen view with multiple nodes. As you can see, there is already less space available. The nodes can also have titles of different lengths, which can make the state badges unequal in length. The configuration containers have unequal height and width.

This is one of the reasons why we remove the titles of the nodes in the mockup. The small status badges can still contain a link to the node and have a tooltip that shows the name of the node. A display that hangs on the wall and provides a full screen view can show you the current status of the nodes in all given configurations. To get more information, the user can open this view on a local device and perform further investigation.

slalomsk8er · 2023-08-01T11:02:14Z

@sukhwinder33445 then we have a problem as showing the titles of the business processes is the whole point of this sponsored feature!

This is what it actually looks in production:

It maybe not perfect but works well for us and as far as I know, we're currently the only users so please don't let the perfect be the enemy of the good.

nilmerg

Once both review requests are resolved, we will merge this.

module.info

library/Businessprocess/Web/Component/BpDashboardFullscreenTile.php

library/Businessprocess/Web/Component/DashboardFullscreen.php

public/css/module.less

nilmerg · 2024-07-25T12:48:47Z

And please rebase with main, otherwise the checks will not run.

lippserd · 2024-10-25T12:02:00Z

@NavidSassan PR will be reviewed next week. Next time, please re-request review or leave a comment and don't resolve conversations. We do not receive notifications of code changes and it is the responsibility of who started the conversation to actually resolve it.

NavidSassan · 2024-10-25T12:03:40Z

Oh sorry, wasn't aware of that. Thanks for the update 👍

lippserd · 2024-10-25T12:04:30Z

Oh sorry, wasn't aware of that. Thanks for the update 👍

No worries!

lippserd

@NavidSassan if you need help with the changes requested, please let me know.

lippserd · 2024-11-06T10:08:49Z

application/controllers/IndexController.php

-        $this->setAutorefreshInterval(15);
+
+        /** View */
+        $view = $this->view;


I would not introduce an extra local variable. Just call $this->view->... twice.

lippserd · 2024-11-06T10:22:29Z

application/controllers/IndexController.php

+        if ($view->showFullscreen) {
+            $this->content()->add(DashboardFullscreen::create($this->Auth(), $this->storage()));
+            $this->setAutorefreshInterval(120);
+            $this->controls()->getAttributes()->add('class', 'want-fullscreen');


This is not required and could be a leftover from previous changes. At least I can't find any CSS introduced by this PR that requires this class to be set.

lippserd · 2024-11-06T10:22:58Z

library/Businessprocess/Web/Controller.php

@@ -49,7 +49,6 @@ public function init()
        $this->view->showFullscreen
            = $this->showFullscreen
            = (bool) $this->_helper->layout()->showFullscreen;
-


Please drop that change.

lippserd · 2024-11-07T08:48:42Z

library/Businessprocess/Web/Component/BpDashboardFullscreenTile.php

+{
+    protected $tag = 'div';
+
+    protected $defaultAttributes = ['class' => 'dashboard-tile dashboard-tile-fullscreen'];


There is no CSS for the dashboard-tile-fullscreen class, so I think it can be safely removed.

lippserd · 2024-11-07T08:54:17Z

library/Businessprocess/Web/Component/DashboardFullscreen.php

+
+class DashboardFullscreen extends BaseHtmlElement
+{
+    /** @var string */


Please remove that comment. It is not needed.

lippserd · 2024-11-07T10:36:13Z

library/Businessprocess/Web/Component/BpDashboardFullscreenTile.php

+            $attributes['href'] = Url::fromPath($url, $urlParams ?: []);
+        }
+
+        $this->add(Html::tag(


The whole part should be moved to assemble(). Necessary properties should be set here then.

lippserd · 2024-11-07T10:46:23Z

library/Businessprocess/Web/Component/DashboardFullscreen.php

+                MonitoringState::apply($bp);
+            }
+
+            $this->add(new BpDashboardFullscreenTile(


I would adjust the arguments to BpDashboardFullscreenTile as follows:

new BpDashboardFullscreenTile( $bp, $title, $meta->get('Description') )

lippserd · 2024-11-07T10:49:11Z

library/Businessprocess/Web/Component/BpDashboardFullscreenTile.php

+        $this->add(Html::tag(
+            'div',
+            ['class' => 'bp-link', 'data-base-target' => '_main'],
+            Html::tag('a', $attributes)


Instead of using $attributes set it directly, i.e.

$baseUrl = Url::fromPath('businessprocess/process/show', ['config' => $bp->getName()]); ... ['href' => $baseUrl]

lippserd · 2024-11-07T10:49:46Z

library/Businessprocess/Web/Component/DashboardFullscreen.php

+                $bp = $storage->loadProcess($name);
+            } catch (Exception $e) {
+                $this->add(new BpDashboardTile(
+                    new BpConfig(),


Use (new BpConfig())->setName($name) so that it works with the requested changes from above and adjust the arguments, i.e.

new BpDashboardFullscreenTile( (new BpConfig())->setName($name), $title, sprintf(mt('businessprocess', 'File %s has faulty config'), $name . '.conf') )

Also note the t/mt change.

lippserd · 2024-11-07T12:00:03Z

library/Businessprocess/Web/Component/BpDashboardFullscreenTile.php

+            $tiles->add(Html::tag(
+                'a',
+                [
+                    'href'  => Url::fromPath($url, $urlParams ?: [])->with(['node' => $node->getName()]),


With the requested changes from above you can use $baseUrl->with(...);

cla-bot bot added the cla/signed label Sep 29, 2022

nilmerg added this to the 2.5.0 milestone Jul 11, 2023

nilmerg assigned NavidSassan Jul 24, 2023

nilmerg requested a review from sukhwinder33445 July 27, 2023 11:01

sukhwinder33445 requested changes Jul 31, 2023

View reviewed changes

nilmerg removed this from the 2.5.0 milestone Aug 9, 2023

nilmerg requested changes Jul 25, 2024

View reviewed changes

lippserd added the dev-call Issues and Pull Requests to be discussed at the Dev Call. label Aug 3, 2024

NavidSassan added 2 commits August 20, 2024 11:07

Add showFullscreen and showCompact for dashboard

e148c11

adjust according to reviews

0feceaf

NavidSassan force-pushed the feature/dashboard branch from 97f0522 to 0feceaf Compare August 20, 2024 11:27

adjust according to review

f4928f2

lippserd self-requested a review October 25, 2024 12:02

lippserd requested changes Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add showFullscreen and showCompact for dashboard #356

Add showFullscreen and showCompact for dashboard #356

NavidSassan commented Sep 29, 2022

cla-bot bot commented Sep 29, 2022

NavidSassan commented Sep 29, 2022

bobapple commented Sep 29, 2022

sukhwinder33445 left a comment •

edited

Loading

sukhwinder33445 Jul 24, 2023

NavidSassan Aug 20, 2024

sukhwinder33445 Aug 20, 2024

slalomsk8er commented Jul 31, 2023

sukhwinder33445 commented Aug 1, 2023 •

edited

Loading

slalomsk8er commented Aug 1, 2023

nilmerg left a comment

nilmerg commented Jul 25, 2024

lippserd commented Oct 25, 2024

NavidSassan commented Oct 25, 2024

lippserd commented Oct 25, 2024

lippserd left a comment

lippserd Nov 6, 2024

lippserd Nov 6, 2024

lippserd Nov 6, 2024

lippserd Nov 7, 2024

lippserd Nov 7, 2024

lippserd Nov 7, 2024

lippserd Nov 7, 2024 •

edited

Loading

lippserd Nov 7, 2024

lippserd Nov 7, 2024 •

edited

Loading

lippserd Nov 7, 2024

Add showFullscreen and showCompact for dashboard #356

Are you sure you want to change the base?

Add showFullscreen and showCompact for dashboard #356

Conversation

NavidSassan commented Sep 29, 2022

cla-bot bot commented Sep 29, 2022

NavidSassan commented Sep 29, 2022

bobapple commented Sep 29, 2022

sukhwinder33445 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slalomsk8er commented Jul 31, 2023

sukhwinder33445 commented Aug 1, 2023 • edited Loading

slalomsk8er commented Aug 1, 2023

nilmerg left a comment

Choose a reason for hiding this comment

nilmerg commented Jul 25, 2024

lippserd commented Oct 25, 2024

NavidSassan commented Oct 25, 2024

lippserd commented Oct 25, 2024

lippserd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lippserd Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lippserd Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukhwinder33445 left a comment •

edited

Loading

sukhwinder33445 commented Aug 1, 2023 •

edited

Loading

lippserd Nov 7, 2024 •

edited

Loading

lippserd Nov 7, 2024 •

edited

Loading