Fixes visual bug by making the icon grey only on hover and white otherwise. #25023
Conversation
|
cc @nextcloud/designers |
MorrisJobke
added
3. to review
|
/backport to stable20 |
|
/backport to stable21 |
|
I would like to get feedback from @nextcloud/designers on this one |
MorrisJobke
changed the title
core/css/icons.scss
Outdated
| .icon-file-white, | ||
| .icon-filetype-text, |
There was a problem hiding this comment.
Hey! We have a dedicated function to automatically change the colours of svg.
You can use the same @include icon-color('text', 'filetypes', $color-white, 1, true); like we do here for example:
Lines 266 to 269 in 4f90766
There was a problem hiding this comment.
That way you do not have to ship another white svg.
|
Sorry for the time it took, could anyone review it please? |
core/img/filetypes/text-white.svg
Outdated
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" standalone="no"?> | |||
There was a problem hiding this comment.
This file is then also not needed anymore, right?
There was a problem hiding this comment.
Sorry, I forgot to remove the file but yes, it is not needed anymore. Should I make another change?
There was a problem hiding this comment.
Yep - just remove it. You could also use --amend to add changes to your last commit and then force push via git push --force-with-lease (this will not overwrite if there were changes on the server inbetween your last pull and your push).
| @@ -92,7 +92,7 @@ $invert: luma($color-primary) > 0.6; | |||
| } | |||
|
|
|||
| /* Colorized svg images */ | |||
| .icon-file, .icon-filetype-text { | |||
| .icon-file, .icon-filetype-text, .icon-file-white:hover, .icon-file-white:focus { | |||
There was a problem hiding this comment.
This does not seem to be the right place for just the focus styling. This should only apply for the button so it should rather be part of settings.scss if even needed as the button should have a focus/hover feedback already.
| .icon-file-white, | ||
| .icon-filetype-text-white { | ||
| @include icon-color('text', 'filetypes', $color-white, 1, true); | ||
| } | ||
|
|
There was a problem hiding this comment.
-white is not ideal as the button will have a primary color adapted to the theming, so whenever there is a bright theme used the white fixed does not fit anymore. I'd suggest something like this:
| .icon-file-white, | |
| .icon-filetype-text-white { | |
| @include icon-color('text', 'filetypes', $color-white, 1, true); | |
| } | |
| .icon-file-primary, | |
| .icon-filetype-primary { | |
| @include icon-color('text', 'filetypes', $color-primary-text, 1, true); | |
| } | |
|
closing for inactivity, does not fit to current code base either |
As described in #24450, the icon looks off in grey but, if just changed to white, it would be a white icon on white background on hover, which I solved by making the icon grey only on hover and otherwise white.