Skip to content
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.

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

NC 25 - Theme-Colored Folder-Icons #34654

Closed
Tracked by #1280
SephGER opened this issue Oct 18, 2022 · 13 comments · Fixed by #35182
Closed
Tracked by #1280

NC 25 - Theme-Colored Folder-Icons #34654

SephGER opened this issue Oct 18, 2022 · 13 comments · Fixed by #35182

Comments

@SephGER
Copy link

SephGER commented Oct 18, 2022

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.
In the new NC25-Theme the thumbnail foldericons have a static greyish color. With this change the themes primary color could be used.

Describe the solution you'd like
Switch from the background-image css property to a combination of mask-image and background-color

Describe alternatives you've considered
Having multiple icon-sets which could be chosen

Drawbacks
If this is used globally for all icons the mime-type specific css would need additional background-color properties either fixed or via theme settings

Additional context

mask-image: url(/apps/theming/img/core/filetypes/folder.svg?v=16);
background: var(--color-primary);
mask-size: contain;

Right now:
Screenshot 2022-10-18 at 14-31-34

With mask-image:
Screenshot 2022-10-18 at 14-32-09

CSS-Change in Browser:
Screenshot_20221018_143243

@SephGER SephGER added 0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement labels Oct 18, 2022
@Jerome-Herbinet
Copy link
Member

Probably the same problem as the one I detailed in this issue : #34578

@szaimen
Copy link
Contributor

szaimen commented Oct 18, 2022

I can confirm this issue:
image

@szaimen szaimen added bug design Design, UI, UX, etc. 25-feedback ui-refresh-feedback and removed enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Oct 18, 2022
@szaimen
Copy link
Contributor

szaimen commented Oct 18, 2022

cc @nextcloud/server-frontend

@SephGER
Copy link
Author

SephGER commented Oct 19, 2022

Probably the same problem as the one I detailed in this issue : #34578

I just tried an reset the theme color to the default and now the folder color changed to another color that suits the bg-image. The #396475 color from before was after upgrading from 24 and having a custom color set in the global design settings. After setting the design color to a custom value again the folder color keeps the last color that was applied by the bg-image.

So it should either always be set with the bg-image and/or give the admin the possibility to set it to a static theme-color (or even a custom color field alongside the theme color?)

@szaimen
Copy link
Contributor

szaimen commented Nov 8, 2022

@nextcloud/frontenders do you have some pointers on how to fix this?

@skjnldsv
Copy link
Member

skjnldsv commented Nov 8, 2022

@nextcloud/frontenders do you have some pointers on how to fix this?

Yes, because the user can also change the colour now, the cache buster needs to be using the user cache buster alongside the admin
See

$cacheBuster = $this->config->getAppValue('theming', 'cachebuster', '0');
if ($this->userId !== null) {
// need to bust the cache for the CSS file when the user background changed as its
// URL is served in those files
$userCacheBuster = $this->config->getUserValue($this->userId, Application::APP_ID, 'userCacheBuster', '0');
$cacheBuster .= $this->userId . '_' . $userCacheBuster;
}

@skjnldsv
Copy link
Member

skjnldsv commented Nov 8, 2022

Maybe having some methods to get cachebuster would be nice to have.

@szaimen szaimen added the 1. to develop Accepted and waiting to be taken care of label Nov 8, 2022
@szaimen
Copy link
Contributor

szaimen commented Nov 8, 2022

Thanks @skjnldsv ! :)
Do you also have a pointer where the folder icons get generated or overwritten? I guess this is where the method then could get used as well to fix this issue.

Also cc @Pytal on this :)

@skjnldsv
Copy link
Member

Do you also have a pointer where the folder icons get generated or overwritten

no clue, would need to be traced back from the front

@juliusknorr
Copy link
Member

The icon is generated in

public function getThemedIcon(string $app, string $image): Response {

The URL generation and adding the cache buster is done here:

if (strpos($image, 'filetypes/') === 0 && file_exists(\OC::$SERVERROOT . '/core/img/' . $image)) {
$route = $this->urlGenerator->linkToRoute('theming.Icon.getThemedIcon', ['app' => $app, 'image' => $image]);
}
if ($route) {
return $route . '?v=' . $cacheBusterValue;

@szaimen
Copy link
Contributor

szaimen commented Nov 15, 2022

@juliushaertl since you are familiar with this part of the code, could you maybe take a crack at this? Otherwise I will try :)

@juliusknorr
Copy link
Member

I won't have time to dive further into this but a few more hints.

It seems the cache buster for mimetype icons is rather added here in the frontend:
https://github.com/nextcloud/server/pull/840/files#diff-d449dd50c36a47c4712a44ea7d00bdc21e58449f69f5d3c28c7fbc61c73146c7R109

  • Make sure that the cache buster is extended with the user cache buster (like
    $userCacheBuster = $this->config->getUserValue($this->userId, Application::APP_ID, 'userCacheBuster', '0');
    $cacheBuster .= $this->userId . '_' . $userCacheBuster;
    )
  • Properly pick the user color when generating the themed icon in
    $color = $this->util->elementColor($this->themingDefaults->getColorPrimary());
    (this should already be the case with the getColorPrimary call

@szaimen
Copy link
Contributor

szaimen commented Nov 15, 2022

I made it work now with #35182. However I am really unsatisfied how I implemented the details and would need some guidance or maybe @skjnldsv or @come-nc how to improve my code. @juliushaertl could you please give me some guidance on that? Thank you :)

@szaimen szaimen added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants