Added GTM installation guide in no-data and tracking code pages #20577
Conversation
plugins/SitesManager/Controller.php
Outdated
| @@ -229,6 +229,7 @@ public function siteWithoutDataTabs() | |||
| 'tagManagerActive' => $tagManagerActive, | |||
| 'consentManagerName' => false, | |||
| 'cloudflare' => $this->siteContentDetector->cloudflare, | |||
| 'isCloud' => Manager::getInstance()->isPluginActivated('Cloud') | |||
There was a problem hiding this comment.
@tsteur This check should be fine to proceed ?
There was a problem hiding this comment.
Personally I think we should try to avoid any code in core that is cloud only. If we need to replace some parts on cloud we can use e.g. events that allow a plugin to overwrite / replace certain parts. That way we can then replace e.g. the GTM intro within the cloud plugin.
And I guess at some point I would also start to refactor all that code again as having one big template that handles everything is a bit messy. Having e.g. separate classes for each type of detection might be smarter, easier to understand and better testable. Those classes could then have e.g. a method renderTab that allows providing additional tabs for the page (where needed) or a method to add sections to the other ways tab or similar. These methods could then have an event that allows to change/replace the content the method returns to make it adjustable on cloud.
There was a problem hiding this comment.
@sgiehl Removed isCloud and started using postEvent to override in other plugins
There was a problem hiding this comment.
@sgiehl Does this approach sounds good to you ?
Similarly can split it for Cloudflare and others in future
There was a problem hiding this comment.
As mentioned in #20577 (comment) I guess I would have solved that a different way. The implementation of the no data screen in general is currently already a bit messy and we are only adding more and more messy code to it. I would have directly refactored that all so it's more flexible and would make it easier for plugins to hook into in order to add new tabs or change the content of certain tabs.
This implementation will now only allow to replace the content of GTM tab, but that might be enough for now. We nevertheless should consider refactoring all that stuff again.
@Stan-vw @mattab One general thing that came to my mind when viewing the no data screen, which is unrelated to this PR:
The Email these instructions button is currently always visible when switching through the tabs. But the instructions that would be send are actually always the same and a mixture of the contents of all tabs (except GTM and Cloudflare). I would have actually expected that when clicking the button while being on the GTM tab, that the instructions for GTM would be included and not the general instructions.
| @@ -12,6 +12,9 @@ | |||
| <li class="tab col {{ columnClass }}"><a {% if siteType != constant('Piwik\\Plugins\\SitesManager\\SitesManager::SITE_TYPE_UNKNOWN') and (consentManagerName == false) and (activeTab == '') %} class="active" {% endif %} href="#integrations">{{ 'SitesManager_Integrations'|translate }}</a></li> | |||
| <li class="tab col {{ columnClass }}"><a {% if (siteType == constant('Piwik\\Plugins\\SitesManager\\SitesManager::SITE_TYPE_UNKNOWN') and (activeTab == '')) or consentManagerName %} class="active" {% endif %} href="#tracking-code">{{ 'CoreAdminHome_TrackingCode'|translate }}</a></li> | |||
| <li class="tab col {{ columnClass }}"><a href="#mtm">{{ 'SitesManager_SiteWithoutDataMatomoTagManager'|translate }}</a></li> | |||
| {% if gtmUsed %} | |||
| <li class="tab col s2"><a {% if activeTab == 'gtm' %} class="active" {% endif %} href="#google-tag-manager">{{ 'SitesManager_SiteWithoutDataGoogleTagManager'|translate }}</a></li> | |||
There was a problem hiding this comment.
Note: The s2 class is actually for having 6 columns. So if only GTM or Cloudflare tabs are displayed, there is actually one space free. But as there is no class in materializecss to have 5 columns, we can maybe leave it as is for now...
There was a problem hiding this comment.
Yes there is nothing which which is available
There was a problem hiding this comment.
Thanks for highlighting this @sgiehl. I'm sure we're all in favour of refactoring as we go. I'm not sure if this work was pushed through in too much of a hurry to include it, whether we could've approached it better, etc. We can discuss it in an upcoming retro if desired.
Also agreed on the idea that the email should include at least info regarding the tab that the user is looking at. Let's create a ticket to look at the email holistically, validate what users want/need and improve it accordingly.
There was a problem hiding this comment.
FYI for e-mail we have #DEV-16664 created internally
|
A few minor non-functional notes from me, can't speak too much towards the functional change yet. |
|
@sgiehl I have pinged you on Slack for ideas on refactor and included suggestion by @michalkleiner |
* Added GTM installation guide in no-data and tracking code pages, #PG-2730 * Updated UI test * Started mentioning the tracking code below step4 for onpremise * UI screenshot updated * Textual change * Screenshot updated * Changes to support steps override GTM steps by other plugins * Updated unwanted line * Updated UI screenshot * UI screenshot updated * Formatting changes
* Added GTM installation guide in no-data and tracking code pages, #PG-2730 * Updated UI test * Started mentioning the tracking code below step4 for onpremise * UI screenshot updated * Textual change * Screenshot updated * Changes to support steps override GTM steps by other plugins * Updated unwanted line * Updated UI screenshot * UI screenshot updated * Formatting changes
…) (#20606) * Added GTM installation guide in no-data and tracking code pages, #PG-2730 * Updated UI test * Started mentioning the tracking code below step4 for onpremise * UI screenshot updated * Textual change * Screenshot updated * Changes to support steps override GTM steps by other plugins * Updated unwanted line * Updated UI screenshot * UI screenshot updated * Formatting changes
Description:
Added GTM installation guide in no-data and tracking code pages.
Fixes: #PG-2730
Review