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

Split landing page and album templates #343

Merged
merged 6 commits into from Oct 9, 2018

Conversation

2 participants
@edwinsteele
Copy link
Contributor

edwinsteele commented Oct 1, 2018

(I suggest reviewing by commit, rather than by file - I've intentionally not squashed to help with review - I can squash if you'd like)

The templating logic in the "landing page" (the one that shows all the albums) and the "album page" has very little in common, but the two pages are currently in the same template file, which makes it more difficult to improve and debug. This PR separates the landing page and album templates. Colorbox and photoswipe has the two separate templates, but a basic include means no logic has changed, nor has any template refactoring (I'm not a user of these themes - someone could easily do the refactor, however). The two templates in Galleria have been refactored and actually have separate logic.

I have a followup branch for the galleria theme which performs prefetches of the CSS and JS resources that are used in the album pages from the landing page. This reduces the load time on the album pages. I'll open that PR if this is accepted.

edwinsteele added some commits Mar 4, 2018

Split landing page and album page templates
Album page templates have a single include for the landing page
template, so rendered pages remain unchanged.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 1, 2018

Codecov Report

Merging #343 into master will increase coverage by 0.08%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #343      +/-   ##
=========================================
+ Coverage   87.32%   87.4%   +0.08%     
=========================================
  Files          19      19              
  Lines        1412    1421       +9     
=========================================
+ Hits         1233    1242       +9     
  Misses        179     179
Impacted Files Coverage Δ
sigal/gallery.py 88.83% <100%> (+0.13%) ⬆️
sigal/plugins/media_page.py 100% <100%> (ø) ⬆️
sigal/writer.py 83.05% <85.71%> (+1.23%) ⬆️

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 9c74abd...8f4c31c. Read the comment docs.

@saimn

This comment has been minimized.

Copy link
Owner

saimn commented Oct 3, 2018

Thanks for opening this Edwin, I agree that this would be a nice improvement.

First, to explain why it is like this: this is really a mistake I made a long time ago, of allowing to mix images and sub-directories. So an album page can contain both a list of sub-albums, and a gallery of images. I doubt that this is a widely used "feature", and I thought I'd delete it, but I never took the time to do it. However if we choose to remove this feature, we should add a warning if an album contain both sub-albums and images.

Then, I think there is an issue with your approach, it seems you did not consider the case of sub-albums. There is nothing really different between the landing page and an album with sub-albums. So you should also handle this case (and I would propose to rename landing.html to album_list.html).


@property
def template_file(self):
return "landing.html"

This comment has been minimized.

@saimn

saimn Oct 3, 2018

Owner

Why using a property instead of the class attribute as it was previously ? You could have just used this:

class LandingPageWriter(AbstractWriter):
    template_file = "landing.html"

template_file = 'index.html'
class AbstractWriter(object):
__metaclass__ = abc.ABCMeta

This comment has been minimized.

@saimn

saimn Oct 3, 2018

Owner

__metaclass__ is a Python 2 thing, with Python 3 you have to use class AbstractWriter(metaclass=ABCMeta):. So it is surprising that it works. But anyway this is not needed, I would prefer the class attribute version.

with progressbar(self.albums.values(),
label="%16s" % "Writing files",
item_show_func=log_func, show_eta=False,
file=self.progressbar_target) as albums:
for album in albums:
writer.write(album)
if album.path == ".":

This comment has been minimized.

@saimn

saimn Oct 3, 2018

Owner

Here you should change the test to use album.albums and album.medias.

Albums can only contain images or subalbums
Albums containing subalbums and image will display a warning and only
 display the subalbums
Rename landing.html template to album_list.html
Stop using abstract base class
Use class attribute instead of @Property
@edwinsteele

This comment has been minimized.

Copy link
Contributor

edwinsteele commented Oct 5, 2018

Thanks for the feedback, Simon. I've implemented the changes you requested, and confirmed that albums containing a sub-album and images show a warning, and only show the sub-album in the rendered gallery. I wasn't sure how to document this breaking change for consumption by users. Let me know if you'd like me to do something more about that.

template_file = "media.html"
@property
def template_file(self):
return "media.html"

This comment has been minimized.

@saimn

saimn Oct 8, 2018

Owner

Can you revert this change?

@saimn

This comment has been minimized.

Copy link
Owner

saimn commented Oct 8, 2018

Thanks for the changes, it just needs a minor change and I can merge.

Then I think the themes will need to be updated, though this can come later. One thing about the templates is that I think that we should always have a "base" template so that the different pages share the same structure. Colorbox has already this with https://github.com/saimn/sigal/blob/master/sigal/themes/colorbox/templates/base.html which is extended by the other templates.

@edwinsteele

This comment has been minimized.

Copy link
Contributor

edwinsteele commented Oct 9, 2018

Changes made. Good suggestion re: a base template. I'll follow the lead of Colorbox and split out the base template when I reorganise the galleria theme in the next PR.

@saimn

This comment has been minimized.

Copy link
Owner

saimn commented Oct 9, 2018

Ok merging, thanks!

@saimn saimn merged commit 676badd into saimn:master Oct 9, 2018

3 checks passed

codecov/patch 94.11% of diff hit (target 87.32%)
Details
codecov/project 87.4% (+0.08%) compared to 9c74abd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@saimn saimn added this to the 2.0 milestone Oct 10, 2018

This was referenced Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment