nextcloud / unsplash

πŸ“ΈπŸ”€β˜οΈ Random Nextcloud log in background from Unsplash
https://apps.nextcloud.com/apps/unsplash
GNU Affero General Public License v3.0
92 stars 23 forks source link

Image resolution too small on large screen #102

Closed skjnldsv closed 2 years ago

skjnldsv commented 2 years ago

The url can specify the current screen size like this https://source.unsplash.com/featured/2560x1440?nature,nature

image

We should use that

newhinton commented 2 years ago

@skjnldsv Yes and no. It would be great if we can add this via css, but we cannot provide this by default (at the moment). Especially since i am preparing a bigger release with many more sources for images. We already can select keywords, but not modify the url directly (as to prevent abuse)

Check out the dev-branch, it has a HD version for the url. Review is appreciated ;)

skjnldsv commented 2 years ago

You can provide the url via javascript and css variables --image-background https://github.com/nextcloud/server/blob/07c9bf1adff8a2d10ff774da32c2ddd54fd01923/apps/theming/lib/Themes/DefaultTheme.php#L191-L203 https://stackoverflow.com/a/55270268/3885878

newhinton commented 2 years ago

I would like to not use js for this, i think the solution i have now is quite elegant.

I will rethink this at a later point, but i think it would be more useful if i would implement better customization for the provider in the dev branch. That would solve this issue aswell, as long as the webbrowser does not influence the loaded images.

It would be kinda awesome if the webbrowser would edit the size, to load a perfectly fitting image. However, i dont see how we could implement that for different image provider like wikimedia, or wallhaven.cc

skjnldsv commented 2 years ago

We could also load a bigger image. Honestly, pixelized login screen looks kinda bad :/

skjnldsv commented 2 years ago

Oh, I have an idea, we could use media queries!

@media only screen and (max-width: 1920px) {
    body#body-login {
        background-image: url('https://source.unsplash.com/featured/1920x1080?nature,nature') !important;
    }
}

@media only screen and (min-width: 1921px) and (max-width: 2560px) {
    body#body-login {
        background-image: url('https://source.unsplash.com/featured/2560x1440?nature,nature') !important;
    }
}

@media only screen and (min-width: 2561px) {
    body#body-login {
        background-image: url('https://source.unsplash.com/featured/3840x2160?nature,nature') !important;
    }
}

EDIT: tested and works! :rocket:

skjnldsv commented 2 years ago

https://github.com/nextcloud/unsplash/pull/103

newhinton commented 2 years ago

I still need to figure out how to make this work with the dev-branch. I could merge this into master, but it would be overriden when i release the dev-branch, and then we will have a regression. That's not really optimal.

skjnldsv commented 2 years ago

Well, it depends, what is the goal of the dev branch? There is no descriptions or changelog :)

skjnldsv commented 2 years ago

I could merge this into master, but it would be overriden when i release the dev-branch, and then we will have a regression. That's not really optimal.

Well, it depends, considering this fixes an issue, if merging dev create a regression, then the dev branch is not acceptable. In a release process, we shouldn't block merging a bugfix because something that is coming afterwards is not compatible with it. The issue should be dealt with the dev branch then imho :shrug:

newhinton commented 2 years ago

@skjnldsv I have implemented your media-queries into the dev branch. Can you take a look at it if it works like you wanted?

If it does, i'll merge your PR so we get your contribution into the history, and when i am ready to release dev we have that feature aswell.

skjnldsv commented 2 years ago

Having a rough look, seems ok :)