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
Added custom URL parameters to style the optOut iframe #12472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, great work 👍
I have added a few suggestions for improvement.
</span> | ||
|
||
<span> | ||
Font Size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't clear that the number entered are pixels.
} | ||
} | ||
|
||
if ($cssfontsize && (preg_match("/^[0-9]+[\.]?[0-9]*(px|pt|em)$/", $cssfontsize))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also support %
and rem
(not because they are useful in this context, but it may confuse people if their value isn't accepted).
There are far more valid values, but that should be the most common ones.
https://developer.mozilla.org/de/docs/Web/CSS/font-size
https://developer.mozilla.org/de/docs/Web/CSS/length
throw new \Exception("The URL parameter fontSize value of '$cssfontsize' is not valid. Expected value is for example '15pt', '1.2em' or '13px'.\n"); | ||
} | ||
|
||
if ($cssfontfamily && (preg_match("/^[a-zA-Z-\ ,]+$/", $cssfontfamily))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex doesn't accept quotes ("
) which are quite common as they are needed when the font has a space in it's name.
Example: -apple-system,"Helvetica Neue Light",HelveticaNeue,"Helvetica Neue","Liberation Sans",Roboto,Arial,sans-serif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we checked and it seems browsers accept the font without double quotes as well... Should be OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But without quotes one needs to escape all spaces and special characters (according to https://drafts.csswg.org/css2/fonts.html#value-def-family-name)
And this breaks the feature for people who just paste the font-family from their website and then just see an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added double quote in the accepted chars in: #12494
<div> | ||
<p> | ||
<span> | ||
Font Color: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text needs to be translatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 we'll do it after merging
</div> | ||
</p> | ||
<pre piwik-select-on-focus><iframe style="border: 0; height: 200px; width: 600px;" src="{{ optOutCustomizer.piwikUrl }}index.php?module=CoreAdminHome&action=optOut&language={{ optOutCustomizer.language }}&backgroundColor={{ optOutCustomizer.backgroundColor.substr(1) }}&fontColor={{ optOutCustomizer.fontColor.substr(1) }}&fontSize={{ optOutCustomizer.fontSizeWithUnit }}&fontFamily={{ optOutCustomizer.fontFamily }}"></iframe></pre> | ||
<p ng-bind-html="'CoreAdminHome_OptOutExplanationBis'|translate:'<a href=\'' + optOutCustomizer.piwikUrl + 'index.php?module=CoreAdminHome&action=optOut&language=' + optOutCustomizer.language + '&backgroundColor=' + optOutCustomizer.backgroundColor.substr(1) + '&fontColor=' + optOutCustomizer.fontColor.substr(1) + '&fontSize=' + optOutCustomizer.fontSizeWithUnit + '&fontFamily=' + optOutCustomizer.fontFamily + '\' rel=\'noreferrer\' target=\'_blank\'>':'</a>'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about angular, but maybe there is a way to construct the URL to avoid the duplication.
.gitignore
Outdated
@@ -67,3 +67,4 @@ php_errors.log | |||
/misc/vagrant | |||
/.vagrant | |||
/plugins/.gitignore | |||
iframe.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that change wasn't intentionally.
Excellent work @Jouveer and great little new feature. Users will love the ability to customise the opt-out iframe 🎉 |
Could Or is there any reason to not include numerals? |
@CountCount sure. Proposed a change for that: #12953 |
References: #12455