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

Show large image size by default on mobile devices #4307

Merged
merged 2 commits into from Jul 23, 2020

Conversation

BrokenEagle
Copy link
Collaborator

Fixes #3493.

@BrokenEagle BrokenEagle force-pushed the mobile-mode-default-image-size branch 2 times, most recently from 6c043cc to 9db52a6 Compare June 6, 2020 19:10
@BrokenEagle
Copy link
Collaborator Author

Per the conversation on Discord (05/04/2020, link), I changed this PR to avoid using a cookie.

I modeled this similarly to how post previews are rendered, i.e. using a picture element with 2 source elements.

One item of note, is that on my development instance, when in the desktop view, the image had a tendency to load twice, with the first being either a partial or a full image download. This was also seen when switching between desktop and mobile modes. Additionally, after I enabling cropped thumbnails, the double loading of images was seen in the index view as well. This behavior was only seen on Chrome, and searching the web, I see that that was an issue in the past.

https://stackoverflow.com/questions/33919840/why-does-the-picture-tag-download-an-image-twice

However, replicating the use of the same element structure on the Danbooru site, the same issue was not seen. I also tested it on Testbooru, and it wasn't seen there either. So it seems like that the production site is doing something that the development instance isn't? Or perhaps it could be something else, like the fact that the development instance is on a VM or using a private host IP?

So I'd recommend testing this on Testbooru first, just to make sure that the correct behavior is still seen, and after confirming that, rolling it out to the main site.

@evazion evazion force-pushed the master branch 23 times, most recently from 7e83272 to 889c4ce Compare June 11, 2020 18:31
@BrokenEagle BrokenEagle force-pushed the mobile-mode-default-image-size branch from 9db52a6 to 6a26bcc Compare June 13, 2020 03:58
@BrokenEagle BrokenEagle force-pushed the mobile-mode-default-image-size branch from 6a26bcc to 131e526 Compare June 13, 2020 04:07
@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@evazion evazion force-pushed the master branch 4 times, most recently from 293306c to 52367c7 Compare June 16, 2020 01:21
@lllusion3469
Copy link
Collaborator

lllusion3469 commented Jun 18, 2020

Apparently some people are really interested in being able to see the full resolution on mobile: https://danbooru.donmai.us/forum_topics/9127?page=307#forum_post_168268 (I commented on that specific problem here)

But if they had "Default image width" set to "original" it would

  • still show the large size
  • but not show the "view original" button on the top of the page, I'm fairly sure

So if this <picture> approach is kept, it might be desirable to display this link / message regardless of what "Default image width" is set to on mobile, maybe via css media queries(?)

Edit: possibly not even reusing the current message but creating a new one explaining that the large version is used one mobile with a link to the original which is maybe hidden using css on desktop


Edit: after actually thinking about it a bit, I'm not sure if I agree with not even having an option for this. Sure, you probably don't need some 4000x4000 original on mobile (though my phone can handle it just fine), but 1080p phones are not that uncommon nowadays (nor are phones that go even higher, like the galaxy s7 from 2015) and 850px vs 1080px (1440px) is probably noticable. Note that the 660px in the media query doesn't refer to actual pixels but to CSS pixels.

Edit: maybe <img srcset="..."> (maybe also the sizes="..." attribute) would be the better option? Here's an article explaining <picture> vs <img srcset="...">. MDN <img> documentation, MDN article "Responsive images".

I'm really not sure how this all works but the TL;DR seems to be: <source media> seems to be for when you want to tell the browser what it should choose (may not even be the same image); <img srcset="..."> is for letting the browser choose which image best to use based on some data you provide, viewport width, whether the user wants to save data etc.

But why's it referring to CSS pixels everywhere (except srcset with <width>w) when the actual screen dimensions should also have some relevance? We're talking about actual images that can't just be scaled after all.

@BrokenEagle
Copy link
Collaborator Author

If they want to see the full size, then they can just use the desktop mode instead.

@evazion evazion merged commit e4af499 into danbooru:master Jul 23, 2020
@evazion
Copy link
Member

evazion commented Jul 23, 2020

Not having the "view original" link will be an issue. This will probably still require some Javascript to detect whether we're showing the sample or the original.

Client hints could potentially help here, but they're still experimental. Client hints let the browser tell the server what the screen width is with an HTTP header. This way we could tell server-side how big the screen is, and therefore which image to display.

We might also want to respect the Save Data header here. This indicates that the client want to save bandwidth, and therefore probably would want sample images even in desktop mode.

@BrokenEagle BrokenEagle deleted the mobile-mode-default-image-size branch July 24, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always default to the resized image on mobile mode
3 participants