Skip to content

Comments

Optimize SVG icons#7130

Merged
jancborchardt merged 5 commits intomasterfrom
fix-broken-svg
Nov 14, 2017
Merged

Optimize SVG icons#7130
jancborchardt merged 5 commits intomasterfrom
fix-broken-svg

Conversation

@pixelipo
Copy link
Contributor

@pixelipo pixelipo commented Nov 10, 2017 •
edited
Loading

Ready for review. This is first part of the #4076 (actions icon types).

The whole to-do list has been moved to a more generic issue #5157

@pixelipo
Fixes #4076 - first batch of fixed and re-optimized icons
f6769e2 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo pixelipo added the 2. developing Work in progress label Nov 10, 2017
@pixelipo pixelipo self-assigned this Nov 10, 2017
@pixelipo
Copy link
Contributor Author

pixelipo commented Nov 10, 2017 •
edited
Loading

I've stumbled upon an icon called pause-big.svg - it's the same as pause.svg, but 40% larger (within the same 16px viewBox). Anyone know where (and why) this icon is used? Can we drop it? @nextcloud/designers

P.S. same goes for play-big.svg

@jancborchardt
Copy link
Member

jancborchardt commented Nov 10, 2017

It's used in the Gallery app detail view / slideshow, bottom right.
But yeah, now that we use SVG only (that was a remnant of PNG times) we could simply do it by CSS.

@pixelipo
Copy link
Contributor Author

pixelipo commented Nov 10, 2017

Thanks @jancborchardt - I'll add it to the Todo list or create a new issue to be fixed after merging this

@pixelipo
second batch of fixed and re-optimized icons
4d5490c 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@codecov
Copy link

codecov bot commented Nov 10, 2017 •
edited
Loading

Codecov Report

Merging #7130 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7130      +/-   ##
============================================
+ Coverage     50.74%   50.75%   +<.01%     
- Complexity    24423    24479      +56     
============================================
  Files          1580     1581       +1     
  Lines         93366    93578     +212     
  Branches       1359     1353       -6     
============================================
+ Hits          47379    47493     +114     
- Misses        45987    46085      +98
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/App.php 17.64% <0%> (-9.63%) 3% <0%> (+1%)
apps/user_ldap/lib/LDAP.php 16.66% <0%> (-7.02%) 51% <0%> (ø)
apps/user_ldap/lib/Mapping/AbstractMapping.php 66.34% <0%> (-5.53%) 26% <0%> (+1%)
apps/user_ldap/lib/User/User.php 75.42% <0%> (-5.39%) 123% <0%> (ø)
apps/user_ldap/lib/User/Manager.php 73.49% <0%> (-4.56%) 22% <0%> (ø)
apps/user_ldap/lib/Configuration.php 41.39% <0%> (-0.16%) 66% <0%> (ø)
...ps/user_ldap/composer/composer/autoload_static.php 0% <0%> (ø) 1% <0%> (ø) ⬇️
apps/user_ldap/ajax/wizard.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../user_ldap/composer/composer/autoload_classmap.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/user_ldap/lib/Jobs/Sync.php 26.38% <0%> (ø) 44% <0%> (?)
... and 7 more

@pixelipo
third batch of fixed and re-optimized icons
7914d8c 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo
Actions icons fixed and re-optimized
1bad132 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@MorrisJobke
Copy link
Member

MorrisJobke commented Nov 13, 2017

@pixelipo Is it possible to make smaller PRs out of this? Otherwise we would end up with a diff that is quite big.

@pixelipo
Copy link
Contributor Author

pixelipo commented Nov 13, 2017

@MorrisJobke diff is kind of useless for testing this PR.

I have finished fixing actions icons so I can stop at this point and create new PRs for other icon types. Changes so far are only those required to make sure that the code is valid and the icons fit the predefined requirements (16*16 size) plus some "weight-loss" (the whole folder is down to 39kB from ~76kB).

The only ways to test this are:

  1. Open icon in Inkscape to make sure it is not "scrambled"
  2. check if there are any broken icons in various components and apps

I can cut it into several smaller PRs you think it will help - but tell me how much files per PR?

@MorrisJobke
Copy link
Member

MorrisJobke commented Nov 13, 2017

I can cut it into several smaller PRs you think it will help - but tell me how much files per PR?

I guess the amount in here is enough. Checking already ~100 icons could be hard. And that's the reason I asked to split it. Dumping more stuff into it would not make it easier to review. ;)

@pixelipo pixelipo added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 13, 2017
This was referenced Nov 13, 2017
Closed
Closed
@jancborchardt
Copy link
Member

jancborchardt commented Nov 14, 2017

Good stuff - @nextcloud/designers let's test and review this! :) Also make sure to open in your SVG editor of choice (e.g. Inkscape) to check they are not mangled anymore.

MorrisJobke
MorrisJobke requested changes Nov 14, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Beside the one icon it looks good 👍

core/img/actions/settings-white.svg Outdated
@@ -1 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" height="16" width="16" version="1"><path d="M6.938 0A.43.43 0 0 0 6.5.438v1.25a5.818 5.818 0 0 0-1.53.656l-.907-.906a.436.436 0 0 0-.625 0l-1.5 1.5a.436.436 0 0 0 0 .624l.906.907c-.285.48-.514.976-.656 1.53H.938a.43.43 0 0 0-.438.438v2.125C.5 8.81.69 9 .938 9h1.25a5.82 5.82 0 0 0 .656 1.53l-.907.908a.436.436 0 0 0 0 .625l1.5 1.5c.176.176.45.176.625 0l.907-.907c.48.285.976.514 1.53.656v1.25c0 .25.19.438.437.438h2.125a.43.43 0 0 0 .438-.438v-1.25a5.82 5.82 0 0 0 1.53-.657l.907.907c.176.175.45.175.625 0l1.5-1.5a.436.436 0 0 0 0-.625l-.906-.906A5.79 5.79 0 0 0 13.812 9h1.25a.43.43 0 0 0 .438-.438V6.437A.43.43 0 0 0 15.062 6h-1.25a5.79 5.79 0 0 0-.656-1.532l.906-.906a.436.436 0 0 0 0-.625l-1.5-1.5a.436.436 0 0 0-.625 0l-.906.906a5.816 5.816 0 0 0-1.53-.656V.437A.43.43 0 0 0 9.063 0zM8 4.157a3.344 3.344 0 0 1 0 6.686 3.344 3.344 0 0 1 0-6.686z" display="block" fill="#fff"/></svg>
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" height="16" width="16" viewBox="0 0 16 16"><path opacity="#fff" d="m6.938 0.5a0.43 0.43 0 0 0 -0.438 0.438v1.25a5.818 5.818 0 0 0 -1.53 0.656l-0.907-0.906a0.436 0.436 0 0 0 -0.625 0l-1.5 1.5a0.436 0.436 0 0 0 0 0.624l0.906 0.907c-0.285 0.48-0.514 0.976-0.656 1.53h-1.25a0.43 0.43 0 0 0 -0.438 0.438v2.125c0 0.248 0.19 0.438 0.438 0.438h1.25a5.82 5.82 0 0 0 0.656 1.53l-0.907 0.908a0.436 0.436 0 0 0 0 0.625l1.5 1.5c0.176 0.176 0.45 0.176 0.625 0l0.907-0.907c0.48 0.285 0.976 0.514 1.53 0.656v1.25c0 0.25 0.19 0.438 0.437 0.438h2.125a0.43 0.43 0 0 0 0.438 -0.438v-1.25a5.82 5.82 0 0 0 1.53 -0.657l0.907 0.907c0.176 0.175 0.45 0.175 0.625 0l1.5-1.5a0.436 0.436 0 0 0 0 -0.625l-0.906-0.906a5.79 5.79 0 0 0 0.657 -1.531h1.25a0.43 0.43 0 0 0 0.438 -0.438v-2.125a0.43 0.43 0 0 0 -0.438 -0.437h-1.25a5.79 5.79 0 0 0 -0.656 -1.532l0.906-0.906a0.436 0.436 0 0 0 0 -0.625l-1.5-1.5a0.436 0.436 0 0 0 -0.625 0l-0.906 0.906a5.816 5.816 0 0 0 -1.53 -0.656v-1.25a0.43 0.43 0 0 0 -0.438 -0.437zm1.062 4.157a3.344 3.344 0 0 1 0 6.6861 3.344 3.344 0 0 1 0 -6.6861z"/></svg>
Copy link
Member

@MorrisJobke MorrisJobke Nov 14, 2017

Choose a reason for hiding this comment

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

This icon is somehow black now (instead of the white version) - it's the top right menu icon.

Copy link
Contributor Author

@pixelipo pixelipo Nov 14, 2017

Choose a reason for hiding this comment

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

Ooops 😱 fixed now 👼

@pixelipo
Fix settings-white icon
a6197a2 
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@MorrisJobke
Copy link
Member

MorrisJobke commented Nov 14, 2017

@nextcloud/designers please review

jancborchardt
jancborchardt approved these changes Nov 14, 2017
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks a lot @pixelipo

@jancborchardt jancborchardt merged commit 346dbfa into master Nov 14, 2017
@jancborchardt jancborchardt deleted the fix-broken-svg branch November 14, 2017 14:51
@jancborchardt
Copy link
Member

jancborchardt commented Nov 14, 2017

I just have two follow-up questions:

  • I saw the strange icons user-plus and user-times before. Haven’t found where/if they are used. Can we delete them?
  • What do we need the PNG icons for? They used to be IE8 fallbacks, but we don’t need that anymore? Is it for notification emails?

@pixelipo
Copy link
Contributor Author

pixelipo commented Nov 14, 2017 •
edited
Loading

@jancborchardt I noticed those as well and haven't touched them. I also couldn't find them used anywhere else and frankly, they shouldn't be, since they are are completely different to user.svg. Perhaps in some chat app? @nextcloud/chat

PNG files are called in places like https://github.com/nextcloud/server/blob/master/apps/comments/lib/Activity/Provider.php#L93
I would love to get rid of them (if possible), and I'll create a separate issue/PR to address that. - P.S. added it to my TODO in #5157

@sualko
Copy link
Member

sualko commented Nov 14, 2017

Perhaps in some chat app?

No, not that I know of.

@jancborchardt
Copy link
Member

jancborchardt commented Nov 14, 2017

Ok then feel free to also remove the user-plus and user-times icon in a PR of yours @pixelipo. :)

@skjnldsv
Copy link
Member

skjnldsv commented Nov 14, 2017

Awesome work here @pixelipo !!

@MorrisJobke
Copy link
Member

MorrisJobke commented Nov 14, 2017

PNG files are called in places like https://github.com/nextcloud/server/blob/master/apps/comments/lib/Activity/Provider.php#L93
I would love to get rid of them (if possible), and I'll create a separate issue/PR to address that. - P.S. added it to my TODO in #5157

No - they are needed there. For example on iOS the activity needs to serve PNGs instead of SVGs and some emails also use that AFAIK. cc @nickvergessen

@pixelipo pixelipo mentioned this pull request Nov 15, 2017
Merged
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Nov 16, 2017
@nickvergessen nickvergessen mentioned this pull request Nov 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@eppfel eppfel Awaiting requested review from eppfel

@Espina2 Espina2 Awaiting requested review from Espina2

Assignees

@pixelipo pixelipo

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 13

Development

Successfully merging this pull request may close these issues.

5 participants

@pixelipo @jancborchardt @MorrisJobke @sualko @skjnldsv