Remove svg shadows

[pixelipo]

@nextcloud/designers next step in cleaning up icons.

• drop-shadow is stronger to make white, shaded icons more visible on white backgrounds
• allow 3 extra icon variants (in addition to default black) - white, shadow and white+shadow
• old classes are still working as expected for backwards compatibility (should be deprecated in the future)
• @nextcloud/gallery templates updated to use new classes Remove svg shadows gallery#331
• @nextcloud/talk templates updated to use new classes Remove svg shadows spreed#509
• removed deprecated SVG icons
• migrated apps/gallery/img/toggle.svg to core/img/actions/toggle-background.svg

Fixes #5593

[pixelipo]

Gallery and Talk apps are known to use icons that were affected by this PR;
I've created PRs for them as well:
nextcloud/spreed#509
nextcloud/gallery#331

Other apps should not be affected by this change (but please test if you know of icons being used elsewhere)

[codecov]

Codecov Report

Merging #7319 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7319      +/-   ##
============================================
+ Coverage     50.86%   50.91%   +0.05%     
- Complexity    24538    24698     +160     
============================================
  Files          1584     1586       +2     
  Lines         93794    94113     +319     
  Branches       1358     1361       +3     
============================================
+ Hits          47705    47919     +214     
- Misses        46089    46194     +105

Impacted Files	Coverage Δ	Complexity Δ
...ib/private/DB/QueryBuilder/CompositeExpression.php	50% <0%> (-12.5%)	6% <0%> (+3%)
apps/dav/lib/DAV/Sharing/Plugin.php	60% <0%> (-3.64%)	14% <0%> (ø)
lib/private/RichObjectStrings/Validator.php	82.14% <0%> (-2.86%)	13% <0%> (+9%)
apps/files_external/lib/Command/ListCommand.php	38.62% <0%> (-2.05%)	39% <0%> (ø)
lib/private/Mail/Message.php	79.16% <0%> (-1.97%)	24% <0%> (+7%)
lib/private/Group/MetaData.php	68.91% <0%> (-1.86%)	19% <0%> (ø)
lib/autoloader.php	79.62% <0%> (-1.63%)	27% <0%> (+12%)
lib/private/Diagnostics/Query.php	36.84% <0%> (-1.62%)	8% <0%> (+3%)
lib/private/Route/Router.php	52.14% <0%> (-0.96%)	50% <0%> (+7%)
lib/private/App/CodeChecker/NodeVisitor.php	88.88% <0%> (-0.63%)	54% <0%> (ø)
... and 39 more

[nickvergessen]

I think the play and pause icons are also used by the audio player app (dont remember the name).

Would also be nice to not remove icons in the late beta/rc phase.
Devs already submitted updates to the appstore, so they should still work...

[pixelipo]

I generally agree, @nickvergessen

but (there's always a but) 😄

• audio app is using custom icons
• audio, video, screen and fullscreen icons are still backwards-compatible for all apps that are using existing "official" icon classes (defined in global icons.scss)
• ⬆️ do break for apps that have custom classnames, but no app should do things that way...
• view-* icons will break compatibility since they were never defined in icons.scss
• this doesn't have to be merged in 13 if it's too late

If keeping the icons will allow this to be merged in 13, then I will do it right away. So?

[MorrisJobke]

If keeping the icons will allow this to be merged in 13, then I will do it right away. So?

Yes - I would not merge it if it breaks with existing apps. Having the css classes there already would be a nice improvement. So apps can adjust but don't loose support. I would say, that dropping of the images is fine for 14 only.

[pixelipo]

Done, @MorrisJobke @nickvergessen those icons are 90% not used anywhere (so they will not be loaded on pages at all), but it's better to keep them here for safety reasons in NC13.

In general, this is part of the #5157 task. When I'm finished with it, I'll add documentation so it will be official/obligatory from NC14

[nickvergessen]

@pixelipo thanks, the main issue is that our icons have no "defined" public. Some APIs for example use the images directly without CSS, so removing them breaks no matter which fallbacks you add for CSS. If there was no beta out, I'd be fine with dropping images etc. But after a beta people test their apps once and assume they will just continue to work fine ;)

[skjnldsv]

The icon-white filter isn't suitable for buttons with icon and text since it will revert the text too.
The idea is great but we need a workaround for this :)

[pixelipo]

@skjnldsv I'm not sure that is an actual problem. If a text is black, than it means the button has white background, so nobody would use .icon-white on it.

You only need .icon-white icons that are on dark backgrounds, so if that icon happens to be a button with text, then text should also be inverted.

Perfect example is the searchbox in the header. I just did a quick test and it can easily be made to use icon-white property.

[skjnldsv]

@pixelipo Hum, good point. So we should add this in the documentation to explain that you should not change the font colour as everything will be handled by the filter. :)

[jancborchardt]

@pixelipo icon-shadow only makes sense in connection with icon-white anyway, no (or if the icon is white itself, in which case it should have the icon-white class)? So icon-shadow should be a subitem of icon-white, not the other way around. Cause not all white icons need shadow, but all shadowed icons are white.

[pixelipo]

That makes sense, @jancborchardt - I've pushed the simplified CSS definitions.

Please test again, especially in Talk and Gallery apps.

[skjnldsv]

Makes more sense yes! :)

[pixelipo]

Any plans to merge this soon? Please keep in mind that related PR was already merged in Gallery, so that app is broken on master until this is merged, too...

[MorrisJobke]

I fixed a syntax error in the scss. 👍

[skjnldsv]

@MorrisJobke was the test failing before your fix?

[MorrisJobke]

@MorrisJobke was the test failing before your fix?

No - it was a drone issue.