Skip to content
This repository has been archived by the owner on Dec 25, 2023. It is now read-only.

Frontent part of full HiDPI support #37

Merged
merged 3 commits into from
Mar 1, 2019
Merged

Frontent part of full HiDPI support #37

merged 3 commits into from
Mar 1, 2019

Conversation

kamil4
Copy link
Contributor

@kamil4 kamil4 commented Feb 27, 2019

This merge request addresses LycheeOrg/Lychee#198. See LycheeOrg/Lychee#92 for the server part and the overview/motivation of why things are done the way they are.

As implement, support for HiDPI requires extensions to the server API. However, I kept the extensions optional so the new frontend can be used with Lychee v3 without any loss in existing functionality (I only implemented the server changes for the v4).

I thought I would discuss some parts of the patch:

I got rid of build.getThumbnailHtml() because extending it to handle HiDPI small/medium would require something like 8 new arguments. So I replaced it with a much simpler build.getAlbumThumb() that's used just for the square thumbnails in albums view, and the rest of the functionality is inlined in build.photo() since that was the only user of it.

I added a heuristics to the prefetcher to preload the same variant of the image as the one currently displayed. The browser chooses which variant to display based on the window size and the variant sizes passed in srcset and exports the chosen one via the currentSrc DOM property.

When the browser window is resized, the sizes tags of the displayed images need to be recalculated to ensure that the browser uses the optimal variant for rendering. For album view I just extended view.album.content.justify(), but for image view I needed to add a new view.photo.onresize() method since there wasn't one before.

@ildyria
Copy link
Member

ildyria commented Feb 27, 2019

@kamil4 Can you check if the changes I made breaks anything on your side ? I cannot test the retina support.

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@d7415
Copy link
Contributor

d7415 commented Feb 27, 2019

@kamil4 Can you check if the changes I made breaks anything on your side ? I cannot test the retina support.

I think a bunch of those changes are things I was thinking about while. It's still a lot of lines, and passing the whole of data seems messy but I don't see a better way.

@ildyria
Copy link
Member

ildyria commented Feb 27, 2019

The nice thing is because it is backward compatible, you don't need to apply the PR on Lychee-laravel to test the Lychee-v3 behaviour. 👍

@kamil4
Copy link
Contributor Author

kamil4 commented Feb 27, 2019

@ildyria, I share your taste in whitespace 😉

One of your changes broke the image view; my latest commit should fix it.

BTW, I had no idea that you can push commits to my fork. The things you learn... 😲

Also, is there some JS validator you guys use/recommend? I consider myself first and foremost a C programmer so I'm missing that girdle of a picky compiler that would warn me about uninitialized variables and such 😉

@ildyria
Copy link
Member

ildyria commented Feb 27, 2019

@kamil4 I'm using https://www.jetbrains.com/phpstorm/
It has a bunch of auto indent thing and also statically checks for non used variables, and warns you against ==.

@d7415
Copy link
Contributor

d7415 commented Feb 27, 2019

BTW, I had no idea that you can push commits to my fork. The things you learn...

There's a checkbox (default on) when you create a PR to allow the repo owner to do this :)

@ildyria
Copy link
Member

ildyria commented Feb 27, 2019

There's a checkbox (default on) when you create a PR to allow the repo owner to do this :)

But it works the best when you actually create a new branch, because otherwise you have a "master" name collision...

@kamil4
Copy link
Contributor Author

kamil4 commented Mar 1, 2019

I'm a little worried that this is not merged yet LycheeOrg/Lychee#92 is. I never tested "new" server with "old" frontend... Then again, if that combo didn't work, I'm sure we would've heard by now. 😉

Anyway, is something blocking this PR? Without it the new high-res images won't get used for anything...

@ildyria ildyria merged commit a80f259 into LycheeOrg:master Mar 1, 2019
@kamil4 kamil4 deleted the hidpi branch March 2, 2019 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants