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

Override default dashboard background with theming one #23588

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

julien-nc
Copy link
Member

If the override switch is on, the dashboard will look for a default background in "default" directory of dashboard's appdata.

This way the theming app just has to put an image there and toggle the config switch (it's not done yet).

@juliushaertl @jancborchardt Do you think it's a good way to do that?

@jancborchardt
Copy link
Member

Right, so the idea would be that theming has a setting called "Use theming background for Dashboard" and it is then forced and non-changeable by users? Or is it just the default one in that case? (I’d prefer the latter.)

@lazerl0rd
Copy link

lazerl0rd commented Oct 22, 2020

Could you possibly add a "no background" (as in, theme colour) option? That would just consoldate your patch and provide a config that covers all the cases a user themself could also pick from the GUI.

@julien-nc
Copy link
Member Author

Or is it just the default one in that case?

@jancborchardt Yes it would just replace the default one, still letting users change it as usual.

Could you possibly add a "no background" (as in, theme colour) option?

@lazerl0rd Do you mean make the "plain background" as default? Yes it would be nice indeed.

@lazerl0rd
Copy link

@lazerl0rd Do you mean make the "plain background" as default? Yes it would be nice indeed.

Yupp, that's exactly what I meant.

@skjnldsv
Copy link
Member

Ready for reviews?

@julien-nc
Copy link
Member Author

@skjnldsv Still WIP. Dashboard side is ready but additional settings in theming app are still missing.

What do you think about the idea of using appdata to store the image set in theming app? Any other/better idea?

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 31, 2020
@skjnldsv
Copy link
Member

Still WIP

labels and/or draft please :)

What do you think about the idea of using appdata to store the image set in theming app? Any other/better idea?

I'm not opposed to it.
Might even be faster to serve on some filesystems.
But I'm no expert 🙈

@julien-nc julien-nc force-pushed the enh/23558/override-dashboard-default-background branch 2 times, most recently from a990f85 to 282bcf7 Compare November 3, 2020 00:33
@julien-nc julien-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 3, 2020
@julien-nc
Copy link
Member Author

labels and/or draft please :)

@skjnldsv Sorry 😅

So here it is after getting lost in the theming css maze 😁. Dashboard theming setting behaves just like the login image one. Delete button switches to plain color, revert button switches to the clouds image, upload button let admins choose a custom default dashboard background.

If dashboard app is disabled, related theming settings are hidden.
If theming app is disabled, default dashboard background will always be the clouds image.

theming

Incorrect getAppValue default values were fixed too in theming app.

Theming app could be Vuetified ™️ but that's another story.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 🐘

if (background === 'default') {
if (themingDefaultBackground && themingDefaultBackground !== 'backgroundColor') {
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + time
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + time
return generateUrl('/apps/theming/image/dashboardBackground') + '?v=' + window.OCA.Theming.cacheBuster

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thing you mentioned that, there is a mistake at

'cacheBuster' => $this->appConfig->getAppValue(Application::class, 'cachebuster', '0'),

Fixed in this branch. It will most likely solve many late favicon/icon/image refresh problems. Yay!

@juliushaertl
Copy link
Member

Code looks good but I'm a bit hesitant to add yet another setting to the theming app. This is quite a special use case imo and I'd rather just reuse the themed login image and not allow another configuration option here. But up to @jancborchardt to decide.

@julien-nc
Copy link
Member Author

Thanks for the reviews!

I'm a bit hesitant to add yet another setting to the theming app

I agree we can't add too much stuff there. I still like the possibility to set a different default background than the login one. It would feel a bit less crowded if login and dashboard sections would be side by side like that:

plop

@jancborchardt @juliushaertl What do you think?

@jancborchardt
Copy link
Member

I'm a bit hesitant to add yet another setting to the theming app. This is quite a special use case imo and I'd rather just reuse the themed login image and not allow another configuration option here.

Totally agree with @juliushaertl here – especially since there is already the ability to individually select the ones we ship as default Dashboard backgrounds, so it makes sense to just unceremoniously replace the "Default" Dashboard background with the one set by theming. :) (This would btw also help for customers, which gives a unified style by default, and based on that people can customize.)

@julien-nc julien-nc force-pushed the enh/23558/override-dashboard-default-background branch 2 times, most recently from 2c8b952 to 9943855 Compare November 10, 2020 11:38
@julien-nc
Copy link
Member Author

@juliushaertl @jancborchardt Right, new behaviour is: If theming app is disabled or the login image is not set, no effect on the dashboard. Otherwise the dashboard default background is the same as set in theming (image or plain).

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

👍

@juliushaertl
Copy link
Member

Needs a rebase 😉

@julien-nc julien-nc force-pushed the enh/23558/override-dashboard-default-background branch from 9943855 to c84f451 Compare November 12, 2020 22:41
@juliushaertl
Copy link
Member

/compile amend /

fix getAppValue default value in theming app
fix cacheBuster value injection

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the enh/23558/override-dashboard-default-background branch from c84f451 to f5ef2d7 Compare November 13, 2020 06:51
@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 13, 2020
@julien-nc julien-nc changed the title Override default dashboard background with an appconfig switch Override default dashboard background with theming one Nov 13, 2020
@julien-nc julien-nc merged commit 2e91ccc into master Nov 13, 2020
@julien-nc julien-nc deleted the enh/23558/override-dashboard-default-background branch November 13, 2020 11:34
@julien-nc
Copy link
Member Author

Thanks for the reviews.

@juliushaertl
Copy link
Member

/backport to stable20

@ruedigerkupper
Copy link

Currently, dashboard does not look good if theming is enabled and set to a light background. See #28649. Is there a way to have dark text color and dark icons for light backgrounds?

@Volker-K
Copy link

Volker-K commented Nov 2, 2021

Why can't dashboard just use the color scheme for the application bar that every app uses?

@Volker-K
Copy link

Volker-K commented Nov 5, 2021

Too bad that Dashboard even overrides the settings from custom-css-App.

@juliushaertl
Copy link
Member

Please open new issues to discuss such things, commenting on one year old pull request will not help here.

@nextcloud nextcloud locked as too heated and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants