Skip to content
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

Merged
merged 9 commits into from Jan 22, 2018

Conversation

Jouveer
Copy link
Contributor

@Jouveer Jouveer commented Jan 18, 2018

References: #12455

Copy link
Member

@Findus23 Findus23 left a 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:
Copy link
Member

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))) {
Copy link
Member

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))) {
Copy link
Member

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

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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:
Copy link
Member

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.

Copy link
Member

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>&lt;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:'&lt;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\'>':'&lt/a>'">
Copy link
Member

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
Copy link
Member

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.

@Findus23 Findus23 added c: Usability For issues that let users achieve a defined goal more effectively or efficiently. c: Design / UI For issues that impact Matomo's user interface or the design overall. labels Jan 18, 2018
@mattab mattab added this to the 3.3.1 milestone Jan 18, 2018
@mattab
Copy link
Member

mattab commented Jan 19, 2018

@mattab mattab merged commit e3f30fb into matomo-org:3.x-dev Jan 22, 2018
@mattab
Copy link
Member

mattab commented Jan 22, 2018

Excellent work @Jouveer and great little new feature. Users will love the ability to customise the opt-out iframe 🎉

@CountCount
Copy link

Could 0-9 be added to the regex for the font name? We do use fonts like "Humanist777" and it is not allowed with the current regex.
I changed line 238 of OptOutManager to
if ($cssfontfamily && (preg_match('/^[a-zA-Z-0-9\ ,\'"]+$/', $cssfontfamily))) {
and it now works in our installation but I would prefer it not to break on updates.

Or is there any reason to not include numerals?

@sgiehl
Copy link
Member

sgiehl commented May 22, 2018

@CountCount sure. Proposed a change for that: #12953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. c: Usability For issues that let users achieve a defined goal more effectively or efficiently.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants