NEW Showing framework/CMS version on module report #55

ScopeyNZ · 2018-05-22T03:05:24Z

This adds a paragraph to display the version of SilverStripe CMS and Framework on the installed module report

Unfortunately the work done for SS3.7 is not available for providing the version so this looks at the contents of the report to find the relevant versions. It allows for extension in CWP. I tested it with an extension too:

class CwpSiteSummaryExtension extends Extension
{
    public function getVersionModules()
    {
        return array(
            'cwp/cwp' => 'CWP',
            'silverstripe/cms' => 'SilverStripe CMS',
        );
    }
}

Resolves #35
Resolves #60

robbieaverill

Looks good overall! Just a couple of points regarding the extensibility of these changes

robbieaverill · 2018-05-22T03:14:38Z

src/Reports/SiteSummary.php

+
+        $fields->insertAfter('ReportDescription', new LiteralField(
+            'Version', '<p><strong>' . _t(__CLASS__ . '.VERSION', 'Version: ') . $this->resolveCmsVersion() . '</strong></p>'
+        ));


The only concern I have with this is that you're adding the field after the extension point, so the update method won't affect it for other devs who might want to remove or modify this field.

Can you switch to using the beforeUpdateCMSFields callback?

$this->beforeUpdateCMSFields(function (FieldList $fields) { $fields->insertAfter... }); return parent::getCMSFields();

beforeUpdateCMSFields isn't available in SS_Report which doesn't extend DataObject but I can use $this->beforeExtending('updateCMSFields', $callback); I guess?

Perfect, cheers

robbieaverill · 2018-05-22T03:16:49Z

src/Reports/SiteSummary.php

+                'silverstripe/framework' => 'Framework',
+                'silverstripe/cms' => 'CMS',
+            );
+        }


I think it'd be good to provide this array to extensions, rather than define it as a fallback, e.g.:

$versionModules = array(...); $this->extend('updateVersionModules', $versionModules); ...

We could expect extensions to modify the array by reference

robbieaverill · 2018-05-22T04:42:38Z

We also need a CWP pull request for this. I'd suggest you make it against the 1 (next minor) branch of the cwp/cwp module. We need to ensure that the framework version is removed in the CWP extension, so it'd read something like SilverStripe CMS 3.7.0, CWP 1.9.0 if you had that installed.

robbieaverill · 2018-05-22T04:43:14Z

src/Reports/SiteSummary.php

+        $this->beforeExtending('updateCMSFields', function (FieldList $fields) {
+            $fields->insertAfter('ReportDescription', new LiteralField(
+                'Version',
+                '<p><strong>' . _t(__CLASS__ . '.VERSION', 'Version: ') . $this->resolveCmsVersion() . '</strong></p>'


I think we should make this an <h3> tag. There's an h2 in the report heading, so this would make sense as the next semantic step (and might align a little more closely with the designs)

robbieaverill · 2018-05-22T04:57:25Z

We also need this to appear between the actions toolbar (added in #36) and the main GridField rather than above the actions toolbar.

ScopeyNZ · 2018-05-22T23:30:34Z

I have moved the version to be below the action buttons now.

I've also addressed #60 in this PR too.

robbieaverill · 2018-05-22T23:40:20Z

src/Forms/GridFieldHtmlFragment.php

@@ -0,0 +1,35 @@
+<?php
+
+class GridFieldHtmlFragment implements GridField_HTMLProvider


Could you add a phpdoc for this class please to explain what it does? Also I think the "button" references are misleading

robbieaverill · 2018-05-22T23:47:51Z

src/Forms/GridFieldHtmlFragment.php

+
+    /**
+     * @param string Fragment to write the button to.
+     * @param string An HTML fragment to render


Missing PHPDoc variable names

robbieaverill · 2018-05-22T23:48:44Z

src/Reports/SiteSummary.php

        )
            ->getComponentByType(GridFieldExportButton::class)
            ->setExportColumns(
                Package::create()->summaryFields()
            );
        return $grid;
    }
+
+    /**
+     * @return string


Please add a description of what this is doing

NightJar

One minor change, but otherwise looks great :)

NightJar · 2018-05-23T00:40:25Z

src/Reports/SiteSummary.php

+        ];
+        $this->extend('updateVersionModules', $versionModules);
+
+        $records = $this->sourceRecords()->filter('name', array_keys($versionModules));


I believe name should be Name

ScopeyNZ · 2018-05-23T00:52:39Z

Added a CWP PR: silverstripe/cwp#85

robbieaverill · 2018-05-23T01:23:40Z

css/sitesummary.css

@@ -2,6 +2,11 @@
  display:block;
 }

+.cms .ss-gridfield > div.site-summary__clearfix {


We use BEM in order to avoid having this kind of specificity - can you please just reference .site-summary__clearfix instead?

Unfortunately there's a margin being added to .cms .ss-gridfield > div that wins out on specificity. I could reduce the selector to something like .cms div.site-summary__clearfix or I could put an !important in. What do you think?

Use an aside instead? :>

(I'm not sure if I'm being all that serious here, but would that be semantically correct, out of interest?)

Ok in that case keep it as it is. Can you maybe add a wee comment saying that?

Done 👍 . @NightJar Asides should be "tangentially related" so I guess it could be appropriate...

… Framework on the installed module report

robbieaverill · 2018-05-23T02:30:44Z

src/Reports/SiteSummary.php

@@ -67,21 +54,61 @@ public function columns()
    public function getReportField()
    {
        Requirements::css('silverstripe-maintenance/css/sitesummary.css');
-
-        /** @var GridField $gridField */
-        $gridField = parent::getReportField();


Can you please add this doc block back in?

👍 Bad merge conflict resolution I guess :\ - Adding a docblock for the $config var right below it too.

That is less necessary, but won't hurt

robbieaverill · 2018-05-23T03:24:17Z

src/Reports/SiteSummary.php

+            ->getComponentByType(GridFieldExportButton::class)
+            ->setExportColumns(
+                Package::create()->summaryFields()
+            );


These two lines are merge conflict resolution errors - they're on 65/66 now

Yep I'm blind. Thanks

robbieaverill added affects/v3 labels May 22, 2018

robbieaverill suggested changes May 22, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from 2e5878c to 033abfd Compare May 22, 2018 03:36

robbieaverill self-assigned this May 22, 2018

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from 033abfd to d8fde1f Compare May 22, 2018 04:30

robbieaverill suggested changes May 22, 2018

View reviewed changes

robbieaverill removed their assignment May 22, 2018

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch 3 times, most recently from 1ea1b10 to 2b7e20c Compare May 22, 2018 23:30

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from 2b7e20c to de4aac7 Compare May 22, 2018 23:37

robbieaverill suggested changes May 22, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch 2 times, most recently from 9b01050 to a70ffdc Compare May 23, 2018 00:20

NightJar approved these changes May 23, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from a70ffdc to 424af55 Compare May 23, 2018 00:48

robbieaverill suggested changes May 23, 2018

View reviewed changes

ScopeyNZ added 2 commits May 23, 2018 14:06

NEW Adding a paragraph to display the version of SilverStripe CMS and…

128d7cb

… Framework on the installed module report

FIX Removing description and altering version text to be a header block

ca03d54

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch 2 times, most recently from c14c5c2 to e1643e4 Compare May 23, 2018 02:18

robbieaverill suggested changes May 23, 2018

View reviewed changes

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from e1643e4 to f886720 Compare May 23, 2018 02:54

robbieaverill suggested changes May 23, 2018

View reviewed changes

FIX Moving the version tag to below the action buttons

f338089

ScopeyNZ force-pushed the pulls/1.0/show-version-on-module-report branch from f886720 to f338089 Compare May 23, 2018 03:35

robbieaverill approved these changes May 23, 2018

View reviewed changes

robbieaverill merged commit 1ccf490 into bringyourownideas:master May 23, 2018

robbieaverill deleted the pulls/1.0/show-version-on-module-report branch May 23, 2018 03:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Showing framework/CMS version on module report #55

NEW Showing framework/CMS version on module report #55

ScopeyNZ commented May 22, 2018 •

edited

Loading

robbieaverill left a comment

robbieaverill May 22, 2018

ScopeyNZ May 22, 2018

robbieaverill May 22, 2018

robbieaverill May 22, 2018

robbieaverill commented May 22, 2018

robbieaverill May 22, 2018

robbieaverill commented May 22, 2018

ScopeyNZ commented May 22, 2018

robbieaverill May 22, 2018

robbieaverill May 22, 2018

robbieaverill May 22, 2018

NightJar left a comment

NightJar May 23, 2018

ScopeyNZ May 23, 2018

ScopeyNZ commented May 23, 2018

robbieaverill May 23, 2018

ScopeyNZ May 23, 2018 •

edited

Loading

NightJar May 23, 2018 •

edited

Loading

robbieaverill May 23, 2018

ScopeyNZ May 23, 2018

robbieaverill May 23, 2018

ScopeyNZ May 23, 2018

robbieaverill May 23, 2018

robbieaverill May 23, 2018

ScopeyNZ May 23, 2018

		@@ -0,0 +1,35 @@
		<?php

		class GridFieldHtmlFragment implements GridField_HTMLProvider

NEW Showing framework/CMS version on module report #55

NEW Showing framework/CMS version on module report #55

Conversation

ScopeyNZ commented May 22, 2018 • edited Loading

robbieaverill 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

robbieaverill commented May 22, 2018

Choose a reason for hiding this comment

robbieaverill commented May 22, 2018

ScopeyNZ commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NightJar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScopeyNZ commented May 23, 2018

Choose a reason for hiding this comment

ScopeyNZ May 23, 2018 • edited Loading

Choose a reason for hiding this comment

NightJar May 23, 2018 • 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

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

ScopeyNZ commented May 22, 2018 •

edited

Loading

ScopeyNZ May 23, 2018 •

edited

Loading

NightJar May 23, 2018 •

edited

Loading