Skip to content

Proof of concept #5593 - inline SVG icon for icon-contacts #5682

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.

Sign up for GitHub

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

Merged
MorrisJobke merged 2 commits into from
Aug 2, 2017
Merged

Proof of concept #5593 - inline SVG icon for icon-contacts #5682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffcdc96
Proof of concept #5593 - inline SVG icon for icon-contacts
pixelipo Jul 11, 2017
c658862
Replace `include` with `file_get_contents`
pixelipo Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/theming/css/theming.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
filter: invert(1);
filter: progid:DXImageTransform.Microsoft.BasicImage(invert='1');
}
#contactsmenu .icon-contacts {
background-image: url('../../../core/img/places/contacts-dark.svg');
#contactsmenu .icon-contacts svg path {
fill: #000;
}
#settings .icon-settings-white {
background-image: url('../../../core/img/actions/settings-dark.svg');
Expand Down
22 changes: 14 additions & 8 deletions core/css/icons.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,20 @@
background-position: center;
min-width: 16px;
min-height: 16px;
display: inline-flex;
align-self: center;
position: relative;
}

[class^='icon-'] svg,
[class*=' icon-'] svg {
height: 16px;
width: 16px;
vertical-align: -3px; // counter default DOM offset
}

.icon-white svg path {
fill: $color-primary-text;
}

.icon-breadcrumb {
Expand Down Expand Up @@ -445,14 +459,6 @@ img, object, video, button, textarea, input, select {
background-image: url('../img/places/calendar-dark.svg?v=1');
}

.icon-contacts {
Copy link
Member

@nickvergessen nickvergessen Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, css is public api, we can not simply drop them

background-image: url('../img/places/contacts.svg?v=1');
}

.icon-contacts-dark {
background-image: url('../img/places/contacts-dark.svg?v=1');
}

.icon-files {
background-image: url('../img/places/files.svg?v=1');
}
Expand Down
1 change: 0 additions & 1 deletion core/img/places/contacts-dark.svg
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion core/img/places/contacts.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 3 additions & 1 deletion core/templates/layout.user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ class="app-icon"/>
<button class="icon-close-white" type="reset"></button>
</form>
<div id="contactsmenu">
<div class="icon-contacts menutoggle"></div>
<div class="icon-contacts icon-white menutoggle">
<?php echo file_get_contents('core/img/places/contacts.svg'); ?>
Copy link
Member

@MorrisJobke MorrisJobke Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasReschke Security problem? Yes or no?

Copy link
Member

@LukasReschke LukasReschke Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The include here isn't really optimal, it's not a security issue per se at the moment but depends on the include orders etc.

I'd prefer a proper absolute inclusion:

<?php echo file_get_contents(__DIR__ . '/../img/places/contacts.svg'); ?>

Copy link
Contributor Author

@pixelipo pixelipo Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK @LukasReschke - I'll update this when I move on to inlining other SVG icons

</div>
<div class="menu"></div>
</div>
<div id="settings">
Expand Down
4 changes: 2 additions & 2 deletions settings/js/federationsettingsview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@
_setFieldScopeIcon: function(field, scope) {
var $icon = this.$('#' + field + 'form > h2 > span');
$icon.removeClass('icon-password');
$icon.removeClass('icon-contacts-dark');
$icon.removeClass('icon-contacts');
Copy link
Member

@nickvergessen nickvergessen Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not working:

bildschirmfoto vom 2017-08-03 17-20-18

$icon.removeClass('icon-link');
switch (scope) {
case 'private':
$icon.addClass('icon-password');
break;
case 'contacts':
$icon.addClass('icon-contacts-dark');
$icon.addClass('icon-contacts');
break;
case 'public':
$icon.addClass('icon-link');
Expand Down