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

Galleria theme now reads image data from json #312

Merged
merged 3 commits into from
Mar 18, 2018

Conversation

edwinsteele
Copy link
Contributor

Having the template generate json makes it sensitive to whitespace, particularly in the description field, where failure means the gallery doesn’t load (errors in the js console). I’ve looked at ~20 galleries of mine without problems, though.

DOM interactive and document complete are noticeably improved with this change, though the first main image of the gallery is still completing its load at about the same time. To be fair the improvement will likely be more stark on a bigger gallery.

For timing information: https://webpagetest.org/video/compare.php?tests=180307_6H_80670dab108419e4b609df0c1e9a3ca3,180302_9A_ff950fed21454ed20fd5e9750d3157ca

@codecov
Copy link

codecov bot commented Mar 7, 2018

Codecov Report

Merging #312 into master will decrease coverage by 0.61%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
- Coverage    87.9%   87.28%   -0.62%     
==========================================
  Files          20       19       -1     
  Lines        1463     1408      -55     
==========================================
- Hits         1286     1229      -57     
- Misses        177      179       +2
Impacted Files Coverage Δ
sigal/image.py 91.89% <0%> (-2.67%) ⬇️
sigal/plugins/watermark.py 90.69% <0%> (-2.33%) ⬇️
sigal/writer.py 81.81% <0%> (-0.64%) ⬇️
sigal/__init__.py 86.89% <0%> (-0.61%) ⬇️
sigal/utils.py 98% <0%> (-0.34%) ⬇️
sigal/plugins/nomedia.py 89.74% <0%> (-0.26%) ⬇️
sigal/plugins/media_page.py 100% <0%> (ø) ⬆️
sigal/plugins/feeds.py 100% <0%> (ø) ⬆️
sigal/compat.py
sigal/video.py 90.9% <0%> (+0.09%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55399b2...2f66532. Read the comment docs.

@edwinsteele
Copy link
Contributor Author

Please hold off on merging this. I’m seeing text being written over the top of images in some cases.

@edwinsteele
Copy link
Contributor Author

The PR is ok. I suspect a recent update of galleria or leaflet started loading images from data: URLs, which were prohibited by my CSP

this.lazyLoadChunks( 10 );
{% if settings.show_map and album.show_map %}
$(".gallery-map-toggle").click(galleryMapToggle);
{% endif %}
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this necessary or useful ?
Also for me the map does not show up on your branch.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking more in detail I see that it is already done a few lines above. So it should probably not been done twice ?
Actually I wonder if all the map loading could be done later, when clicking on the map button, to avoid loading tiles for nothing. But it's not related to your PR, I can try to address this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The duplication was unintentional. Thanks for catching it. Instead of continuing to use this new show_map conditional, I’ve moved this simulated click into an existing conditional block that achieves the same outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great to avoid unnecessary loading of the maps. There’s quite some data being downloaded in the background. I think the map function will be used infrequently enough that we shouldn’t unconditionally load it in the background.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed!

big: "{{ media.big }}"{% endif %}
{% endif %}
{% if media.type == "video" %}
video: "{{ media.filename }}"
Copy link
Owner

@saimn saimn Mar 10, 2018

Choose a reason for hiding this comment

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

So it seems that thanks to this the /img/empty.png file is not more necessary ? Do you know how galleria proceed for the thumbnail ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t have videos in my regular galleries, but when I did add one a thumbnail was generated, so I’ve made an update to use that thumbnail, and removed /img/empty.png

@saimn
Copy link
Owner

saimn commented Mar 10, 2018

Thanks @edwinsteele , it looks great, except that the map loading seems broken. This will probably be really interesting for big galleries.

Increase thumbnail lazy load chunk size, as only the first chunk
of thumbnails are loaded before the map layers are loaded in the
background. 50 thumbnails should fill the thumbnail row on most
screens, so gives the impression of a complete gallery load.
Also offer a big link for videos, if configured, not just for images
json loading means that static/img/empty.png is unnecessary.
@edwinsteele
Copy link
Contributor Author

except that the map loading seems broken

I’m not exactly sure what you mean by this, but if you mean the duplication, it’s sorted now. Map loading is working for me with these changes.

@saimn
Copy link
Owner

saimn commented Mar 18, 2018

@edwinsteele - In my tests map loading was not working, I'm testing again now.

@saimn
Copy link
Owner

saimn commented Mar 18, 2018

Ok, the map works now (in your previous version it was displayed and hidden because click(galleryMapToggle) was called twice).

@saimn
Copy link
Owner

saimn commented Mar 18, 2018

Looks all good, thanks !

@saimn saimn merged commit 108bba7 into saimn:master Mar 18, 2018
@saimn saimn added this to the 2.0 milestone Mar 19, 2018
@edwinsteele edwinsteele deleted the galleria-json-config branch October 1, 2018 00:44
kontza pushed a commit to kontza/sigal that referenced this pull request Aug 28, 2020
Galleria theme now reads image data from json
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.

None yet

2 participants