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

Add LiveZilla Tag #95

Merged
merged 4 commits into from Nov 4, 2018

Conversation

Projects
None yet
3 participants
@scysys
Contributor

scysys commented Oct 2, 2018

Add LiveZilla Tag

Tested and working.
Using it a few days on own Websites.

I think it has all options that are needed.

@Findus23 Findus23 added the New Tag label Oct 2, 2018

@Findus23

Hi,

many thanks for creating a new Tag.

I have a few comments below (keep in mind that I don't know LiveZilla)

Please also fix the indention as it seems to be a bit off in a few lines.

Show resolved Hide resolved Template/Tag/LivezillaDynamicTag.php Outdated
// return parent::getIcon();
//
// The image should have ideally a resolution of about 64x64 pixels.
return 'plugins/TagManager/images/icons/livezilla_icon.png';

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

I wanted to complain about the ugly icon, but it seems like you took the best they have
🙂

This comment has been minimized.

@scysys

scysys Oct 5, 2018

Contributor

I try to find another Image, but im failed :(

Show resolved Hide resolved Template/Tag/LivezillaDynamicTag.web.js
}),
$this->makeSetting('LivezillaDynamicDomain', '', FieldConfig::TYPE_STRING, function (FieldConfig $field) {
$field->title = 'Livezilla Domain';
$field->description = 'Enter your Domain without http:// or https://';

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

While obviously everyone should be using https, it is a bit counter intuitive that https is hardcoded.

This comment has been minimized.

@scysys

scysys Oct 2, 2018

Contributor

Livezilla is an Chat and Monitoring solution. Thoose things violates privacy of an website visitor. I do not tolerate those implementations over an insecure http connection!

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

I fully agree, then you should make it clear to the user (e.g. make them enter the URL and throw an error if it isn't HTTPS).

If you want to I can help you write this validate function.

$field->description = 'In most cases it should be the Standard. (true)
Available options are: true or false'; // Howto linebreak?
$field->validators[] = new NotEmpty();
$field->validators[] = new CharacterLength(4, 5);

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

Please use a Checkbox for boolean settings instead of saving a string.

See here for an example:

$this->makeSetting('raygunEnablePulse', false, FieldConfig::TYPE_BOOL, function (FieldConfig $field) {
$field->title = 'Enable Pulse (Real User Monitoring)';
$field->uiControl = FieldConfig::UI_CONTROL_CHECKBOX;
$field->description = 'Automatically identify front end performance issues causing slow page load speeds. See what your users see in the browser and discover why users had poor quality experiences.';
})

$field->validators[] = new NotEmpty();
$field->validators[] = new CharacterLength(4, 5);
}),
$this->makeSetting('LivezillaDynamicCookieLawName', '', FieldConfig::TYPE_STRING, function (FieldConfig $field) {

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

I think the cookie feature makes this tag unnecessarily complicated and hard to understand.
@tsteur What do you think about adding a cookie feature (trigger?) to MTM?

This comment has been minimized.

@scysys

scysys Oct 2, 2018

Contributor

I using it with an other implantation for the tag-manager. It respect the Privacy of European Users and load this Tag only when he really accept those parts on an website.

The tag-manager gives me no real way to document this for other users.

if (document.cookie.indexOf('' + LivezillaDynamicCookieLawName + '=' + LivezillaDynamicCookieLawValue + '') > -1 ) {
var s1 = document.createElement("script"), s0 = document.getElementsByTagName("script")[0];
s1.type = 'text/javascript';
s1.defer = LivezillaDynamicDefer;

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

This is the reason you never store boolean values as a string.
Even in Javascript "false" is true/truthy which means that if a user had entered "false" into the settings it would still be deferred.
Because this returns this is truthy

if ("false") {
    console.log("this is truthy")
} else {
    console.log("this is falsey")
}
@@ -278,6 +278,9 @@
"LastUpdated": "Zuletzt aktualisiert",
"LastVersions": "Letzte Versionen",
"LearnMore": "Mehr dazu",
"LivezillaDynamicTagName": "Livezilla Chat / Monitoring",

This comment has been minimized.

@Findus23

Findus23 Oct 2, 2018

Member

Translations are managed via transifex, so please don't modify them here.

This comment has been minimized.

@scysys

scysys Oct 2, 2018

Contributor

I did not knew that.

scysys added some commits Oct 2, 2018

Revert "Add LiveZilla Tag"
This reverts commit 9288123.
$field->validators[] = new NotEmpty();
$field->validators[] = new CharacterLength(32);
}),
$this->makeSetting('LivezillaDynamicDomain', '', FieldConfig::TYPE_STRING, function (FieldConfig $field) {

This comment has been minimized.

@tsteur

tsteur Oct 3, 2018

Member

I'm not quite sure but could this be maybe automatically configured in the JavaScript depending on the current domain? eg var LivezilleDomain = localhost.origin or so?

This comment has been minimized.

@Findus23

Findus23 Oct 4, 2018

Member

@tsteur I am not sure, but as I understood it this is the URL where Livezilla is installed (so similar to the Matomo URL)

This comment has been minimized.

@tsteur

tsteur Oct 4, 2018

Member

I see. Makes sense 👍

@Findus23

Appart from the question on how to best handle opt-out/opt-in cookies, this looks good to me now.

@tsteur, can you please take a closer look.

$this->makeSetting('LivezillaDynamicDefer', true, FieldConfig::TYPE_BOOL, function (FieldConfig $field) {
$field->title = 'Livezilla Script "defer"?';
$field->uiControl = FieldConfig::UI_CONTROL_CHECKBOX;
$field->description = 'In most cases you should let it activated.';

This comment has been minimized.

@tsteur

tsteur Oct 4, 2018

Member

If possible, be good to give an example when it makes sense to disable it. Not needed though.

This comment has been minimized.

@scysys

scysys Oct 5, 2018

Contributor

No its really not needed. But sometimes you have sites where you would remove this part. So there are 2 Options. Remove "defer" completely or leave it as default and give an user no options to remove it.

This comment has been minimized.

@scysys

scysys Oct 5, 2018

Contributor

An really better solution would be an general option in Tag Manager to async or defer scripts. I don't find those options at this moment.

Example from Qubit´s OpenTag Manager
bildschirmfoto 2018-10-05 um 13 51 28

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 4, 2018

@scysys @Findus23 looks good to me. I would only replace all tabs with spaces and sometimes it looks like there are two many tabs/spaces.

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 4, 2018

Tested it and worked fine for me.

@tsteur

tsteur approved these changes Oct 7, 2018

Cheers 👍 I'll merge soon

@tsteur tsteur merged commit 807883a into matomo-org:master Nov 4, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment