Add missing icons and compile them into sprite, closes #56

[raimund-schluessler]

See #56

• add all missing icons
• adjust css files
• optimize svg files
• compile css files into single file (will be done when moving to SCSS, see Cleanup CSS code and move to SCSS #95)

[pixelipo]

@raimund-schluessler svg files could use some cleanup, I noticed a lot of unneeded code. Also, default for Nextcloud is 16x16 size for svg viewbox (20x20 is used for app icons only). If you want, I can create a PR with updated SVGs, but it should be done before sprite is made, because background-positions will be affected.

[raimund-schluessler]

@pixelipo Optimizing the SVGs would be very welcome. However, before optimization I need to add all missing icons first, since some of them are only available in the sprites file anymore.

I will let you know, when I am done so you can optimize the SVGs. About the size I am not sure. They are all 20x20px² and I am not sure if they would all fit into 16x16px².

[raimund-schluessler]

@pixelipo I extracted all icons from the sprite and saved them as separate files. Feel free to have a look and optimize them. But I think some of them didn't take the extraction well and might be pretty broken (although shown correctly). I will maybe need to redraw some of them once everything else is done to get them nice. But maybe you can clean them up anyway, this would be awesome. 😃

I will now focus on adjusting the css file and get everything else nice.

In case you want to compile the sprite file, make should compile it.

[pixelipo]

@raimund-schluessler I'm about 80% done with optimization. This includes resizing svgs, replacing some with the core counterparts and getting rid of few that are not needed.

I would like to replace divider.svg with border-bottom: 1px dotted #b0b0b0 - is that ok with you?

Additionally, I think it would make sense to remove checkbox svgs and use border + checkmark.svg instead - it would look like this:

Lastly, where do you want me to commit this? In a separate branch/PR, in this branch/PR or in a new branch based on this branch?

[raimund-schluessler]

Thanks for optimizing them! 😃 Could you tell me, which ones you replaced/deleted? Some of the currently unused were used for features currently not implemented anymore, but which might come back in future versions (e.g. the clock and toggle icons were used for reminders).

I must admit I am a bit skeptical about the resizing, but lets see how that turns out.

I would like to replace divider.svg with border-bottom: 1px dotted #b0b0b0 - is that ok with you?

Yes, that's fine with me. The difference is not this big.

Additionally, I think it would make sense to remove checkbox svgs and use border + checkmark.svg instead - it would look like this:

Ok as well, although the checkmark is slightly smaller than before. Could be solved by a separate and larger element on top, but maybe not necessary.

Lastly, where do you want me to commit this? In a separate branch/PR, in this branch/PR or in a new branch based on this branch?

Just commit in this branch/PR, we can adjust everything necessary here.

[raimund-schluessler]

Btw. which software do you use for editing and optimizing the SVGs?

[pixelipo]

Inkscape for editing, scour for optimization (same script available in core ).

16px won't affect view at all, but scales better and maintains consistency across Nextcloud

[pixelipo]

Ok, @raimund-schluessler , I've pushed my changes.

There is one known bug: sortorder-dropdown-button button is not displaying correct current sorting - this is because classes are not the same ones created with the sprite-maker. Easy solution would be to add background-position in styles.css for all of those, but it's not a very solid solution because it can easily break every time we update sprite file.

Another known issue is that opacities are not 100% sorted out. Somewhere it's too dark, elsewhere too light, but it shouldn't be too hard to fix.

Please check for other bugs.

I have put all the icons into a new folder src so that it's easier to compare to the old ones and fallback if needed.

Size of sprite.css.svg has been reduced by 90% 😉

I have replaced some of the icons with the ones existing in nextcloud core - notable example is categories, which are swapped with tag.svg which IMO makes more sense.

[pixelipo]

P.S. I've pushed 2 versions of percent and update icons - we need to decide which one looks better.

[raimund-schluessler]

@pixelipo Thanks for all the work, I will have a look as soon as I can.

I already saw, that you adjusted the background-positions ins style.css. This will not work reliable, because the positions may change with every sprite update as well. Means we need to use the generated css file and adjust all class names accordingly. I will take care of this.

[raimund-schluessler]

I had a look at the optimized icons and I am fine with most of the adjustments. However, there are some changes I don't find optimal:

• Icons 1, 3 and 4 have a to high spacing between the lines now and the lines could be slightly thicker.
• Icon 2 is very dark now. Is this a standard Nextcloud icon now?
• For icon 5 the style of the arrow head is off.
• Icon 6 changed quite drastically. It is not symmetric anymore, the width of the two peaks is to low and the linewidth needs to be increased.
• Icon 7 and 9 are missing completely.
• Icon 8 looks quite ugly now.

I would adjust the icons accordingly, but would like to have feedback why this changed. 😉 Regarding the css, I will take care. Some positions of text etc. changed due to the changed icon size.

Also, the stars look quite chubby now, but I guess this is the standard Nextcloud design?!

Also @jancborchardt could have a look.

[raimund-schluessler]

There is one known bug: sortorder-dropdown-button button is not displaying correct current sorting - this is because classes are not the same ones created with the sprite-maker.

Solution is to match classnames and icon file names, will do so.

[pixelipo]

@raimund-schluessler thanks for the feedback. You raise some valid points and I'll try to address them all:

• since this is a WIP, I tried to make as little changes in templates as possible - I figured it's more important to get updated SVGs approved and it will be easy to modify classnames later; you or I can take care of that once final (for now) set of icons is defined
• icons 1, 3 and 4 are derived from a standard Nextcloud menu icon. Perhaps we can discuss updating it in core as well, but I would like to keep consistency
• icon 2 is standard icon in core - I haven't made any changes there; it does have a lot of "filled" space, but I don't think it looks bad in the app itself (with correct opacity set)
• frankly, I'm not 100% happy with the look of the redesigned icon 5 either; I tried to follow some generic rules that seem to be present in other standard icons (rounded corners, 1.5 or 2px stroke lines; please change it as you see fit; by the way, there are 2 versions of it in src; P.S. IMO that icon should look completely different - it doesn't really represent sorting - I would rather use manual.svg instead
• regarding icon 6, I see your point; I'll update it tomorrow, ok?
• I removed icon 7 on purpose in order to keep consistent with other parts of Nextcloud; where this icon is used, there are 2 states of toggle - off (opacity 0.5 or lower) and on (opacity 0.8 or 1)
• icon 8 is standard Nextcloud icon; old one is totally off regarding design; I would argue that it could use fatter lines, but that's a discussion for @nextcloud/designers
• I didn't add icon 9 because I don't know where it is used - please show me an example so I can figure it out
• also missing is the "random" icon, used for color-selector; should I add it to src and sprite as well?
• regarding icon size, I think new ones take up about the same space (pixels) as the old ones; size can be easily changed like I did for the star in the app-sidebar header:

#content .icon.detail-star {
  height: 24px;
  width: 24px;
  background-size: 144px 144px;
}

• starts are a bit chubbier, but that is Nextcloud standard icon

[pixelipo]

if this is used for icons where no background should be shown, I propose background-image: unset; as a more permanent solution

[raimund-schluessler]

Yes. I need to cleanup css anyway, see #95 .

[raimund-schluessler]

since this is a WIP, I tried to make as little changes in templates as possible - I figured it's more important to get updated SVGs approved and it will be easy to modify classnames later; you or I can take care of that once final (for now) set of icons is defined

Yes, that's fine. I will take care and adjust everything.

icons 1, 3 and 4 are derived from a standard Nextcloud menu icon. Perhaps we can discuss updating it in core as well, but I would like to keep consistency

Ok, then lets keep it consistent and maybe create a core issue.

icon 2 is standard icon in core - I haven't made any changes there; it does have a lot of "filled" space, but I don't think it looks bad in the app itself (with correct opacity set)

Yes, could work. I maybe just need to get used to it.

frankly, I'm not 100% happy with the look of the redesigned icon 5 either; I tried to follow some generic rules that seem to be present in other standard icons (rounded corners, 1.5 or 2px stroke lines; please change it as you see fit; by the way, there are 2 versions of it in src; P.S. IMO that icon should look completely different - it doesn't really represent sorting - I would rather use manual.svg instead

My bad, this icon was used to reload the tasks inside the app. We do not need it any more, this function will not come back. Was not related to sorting at all.

regarding icon 6, I see your point; I'll update it tomorrow, ok?

Great, thanks!

I removed icon 7 on purpose in order to keep consistent with other parts of Nextcloud; where this icon is used, there are 2 states of toggle - off (opacity 0.5 or lower) and on (opacity 0.8 or 1)

Alright, then I will work with opacity.

icon 8 is standard Nextcloud icon; old one is totally off regarding design; I would argue that it could use fatter lines, but that's a discussion for @nextcloud/designers

I liked the old one better, but consistency > taste I guess. Maybe core issue.

I didn't add icon 9 because I don't know where it is used - please show me an example so I can figure it out

It is currently not used, but it was used to exchange the reminder type. Can't give you a sample at the moment, obviously. Sorry.

also missing is the "random" icon, used for color-selector; should I add it to src and sprite as well?

Thought about that too, might make sense.

regarding icon size, I think new ones take up about the same space (pixels) as the old ones; size can be easily changed like I did for the star in the app-sidebar header:

Roughly, yes. Some icons are slightly smaller, but I guess this is only visible in a direct comparison.

start are a bit chubbier, but that is Nextcloud standard icon

Core issue? 🙈

[pixelipo]

Which percent icon do you prefer - narrow:

or wide?

[MorrisJobke]

Wide

[pixelipo]

OK, I'm done for now. Everything but icons 8 and 9 have been updated/fixed. I propose to ignore icon 9 for now and make something up when the feature returns.Regarding icon 8, two possible solutions:

1. create an issue in core
2. use close.svg in the upper right corner instead, similar to Files and Deck apps

I've prepared a new hide.svg icon which could replace confirm.svg in core - it looks like this:

Compared to current one from core:

I haven't ran sprite script - I've left it to you, @raimund-schluessler because you will be updating CSS.

[raimund-schluessler]

@pixelipo Thanks for your support, the icons look good now. I will adjust the CSS.

[pixelipo]

No problem. I pushed one more small change - sprite svg is now generated in the correct folder (img/ instead of css/svg/)

[raimund-schluessler]

I fixed all issues with the icons, their position is now given by the compiled sprites.css file. I suppose in general, everything should be fine now. In case there are smaller issues, I would like to fix them in follow up PRs, since this one here is already huge.

There is still a lot to do on the css side, it has to be cleaned up. But this I will do when moving to SCSS, otherwhise we will have to do the work twice.

@pixelipo Mind having a look?

[raimund-schluessler]

I just fixed a small javascript error, which I missed before.

[pixelipo]

@raimund-schluessler I will try to review this again as soon as possible, but right off the bat - I don't think it's a good idea to change the icon prefix. App-specific icons should be 100% compatible with core CSS.

[raimund-schluessler]

@pixelipo I would also prefer to have it named icon-* instead, but one server css rule interfered with the icons in the .app-navigation-entry-menu of the sort-order-dropdown. The rule https://github.com/nextcloud/server/blob/a891d24176c4ce17c77e8d0005993a53b5ff6b91/core/css/apps.scss#L705-L713 sets the background-position and I didn't find a way to overwrite it.

Before, the css class setting the icon's background-position was named detail-date, so there was no problem.

Now all the classes would be named icon- and this rule kicks in. I cannot overwrite it, because the background-position of the icons is compiled by svg_sprite, hence a simple !important is not possible.

If you have a better solution for this, feel free to tell me (or just implement it 😉).

[raimund-schluessler]

We could maybe make the CSS icon rule of Tasks more specific than the one of core. But since sprites.css is compiled, this would require SCSS and @extend. I could try that in #108.

[raimund-schluessler]

@pixelipo Please see #108 (comment) for a solution of the ico-*-problem (using SCSS).

[raimund-schluessler]

Just for the record, before:

and after:

After is with #108 applied, but that should not make any major difference.

[raimund-schluessler]

Ok, I will rebase and merge this one here, since we need it for other PRs which are impossible to review at the moment without this one here merged.

I don't really like that, but I think it is the best way at the moment. We can fix everything in follow-up PRs. The ico-* issue will be solve with #108.

Edit: Waiting for @skjnldsv to have a look 😄

[skjnldsv]

All clear! :)

[skjnldsv]

It could be great to have a small function for that, much cleaner! :)

[raimund-schluessler]

Good idea, I created an issue #113 for this to tackle it after all this PR mess 😄

[raimund-schluessler]

@skjnldsv Thanks for reviewing, I will rebase/squash and merge. 👍