Skip to content

Fixes visual bug by making the icon grey only on hover and white otherwise. #25023

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

Closed
RafaOP wants to merge 2 commits into from
+10 −3
Closed

Fixes visual bug by making the icon grey only on hover and white otherwise. #25023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fef42f4
Fixes #24450 by making the icon grey only on hover and white otherwise.
RafaOP Jan 8, 2021
2081939
Fixes #24450 by making the icon grey only on hover and white otherwise.
RafaOP May 10, 2021
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
Prev Previous commit
Fixes #24450 by making the icon grey only on hover and white otherwise.
  • Loading branch information
@RafaOP
RafaOP committed Jul 6, 2021
commit 20819397e96ce3b316506263efc9d0ee7454c9e3
6 changes: 1 addition & 5 deletions apps/theming/css/theming.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,10 @@ $invert: luma($color-primary) > 0.6;
}

/* Colorized svg images */
.icon-file, .icon-filetype-text, .icon-file-white:hover {
.icon-file, .icon-filetype-text, .icon-file-white:hover, .icon-file-white:focus {
Copy link
Member

@skjnldsv skjnldsv May 30, 2021

Choose a reason for hiding this comment

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

@juliushaertl please confirm.
If so, it's good for me

Copy link
Member

@juliusknorr juliusknorr Jul 14, 2021

Choose a reason for hiding this comment

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

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.

background-image: url(./img/core/filetypes/text.svg?v=#{$theming-cachebuster});
}

.icon-file-white, .icon-filetype-text-white {
background-image: url(./img/core/filetypes/text-white.svg?v=#{$theming-cachebuster});
}

.icon-folder, .icon-filetype-folder {
background-image: url(./img/core/filetypes/folder.svg?v=#{$theming-cachebuster});
}
Expand Down
7 changes: 5 additions & 2 deletions core/css/icons.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,12 +384,15 @@ audio, canvas, embed, iframe, img, input, object, video {
}

.icon-file,
.icon-file-white,
.icon-filetype-text,
.icon-filetype-text {
@include icon-color('text', 'filetypes', #969696, 1, true);
}

.icon-file-white,
.icon-filetype-text-white {
@include icon-color('text', 'filetypes', $color-white, 1, true);
}

Comment on lines +391 to +395
Copy link
Member

@juliusknorr juliusknorr Jul 14, 2021

Choose a reason for hiding this comment

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

-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:

Suggested change
.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);
}

.icon-filetype-file {
@include icon-color('file', 'filetypes', #969696, 1, true);
}
Expand Down
9 changes: 0 additions & 9 deletions core/img/filetypes/text-white.svg
View file
Open in desktop

This file was deleted.