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

Implement Galleries as JSON+JS for all themes #1765

Closed
wants to merge 21 commits into from

Conversation

ralsina
Copy link
Member

@ralsina ralsina commented May 28, 2015

Working on #1764 and #1623

Review on Reviewable

@ralsina ralsina changed the title [WIP] Refactor galleries again [WIP] Implement Galleries as JSON+JS for all themes May 29, 2015
@ralsina ralsina changed the title [WIP] Implement Galleries as JSON+JS for all themes Implement Galleries as JSON+JS for all themes May 29, 2015
@ralsina
Copy link
Member Author

ralsina commented May 29, 2015

@Aeyoun this adds JS galleries to base... is that ok with you? Base is your baby :-)

The main goal is to let all the other themes just inherit gallery.tmpl and not have to tweak it.
For example, consider how it looks here https://themes.getnikola.com/v7/lanyon/galleries/demo/index.html ... with these changes it "Just Works".

<meta name="viewport" content="width=device-width">
<title>My Nikola Site | My Nikola Site</title>

<link href="assets/css/rst.css" rel="stylesheet" type="text/css">
Copy link
Contributor

Choose a reason for hiding this comment

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

Type is not needed for text/css.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought some validator or other complained if it was not there?

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, I have no idea why this file is in this diff. It's exactly the same in master :-(

@Kwpolska
Copy link
Member

Kwpolska commented Jun 5, 2015

  1. The way you load jQuery and flowr is hacky, and will not make USE_CDN=False users happy; it would be easier to just put the JS in the usual place for all pages (besides, I already sneaked some JS into base when no-one looked)
  2. The galleries are now completely inaccessible to people with JavaScript disabled; previous versions had graceful fallback with the images still visible (just add the <noscript> bit back in)
  3. Why does base have its own render function? Why are there no such functions in bootstrap and bootstrap3? What would happen if you did not put the function there?

@ralsina
Copy link
Member Author

ralsina commented Jun 5, 2015

@Kwpolska

  1. You mean putting jquery in base and using USE_CDN to load one or the other? Done
  2. I don't much mind that usecase. Sure I can add it, but the web in general is inaccessible with JS disabled. In this specific case, sure, why not. Done
  3. Because base didn't have it before, bootstrap* did: https://github.com/getnikola/nikola/blob/refactor-galleries-gain/nikola/data/themes/bootstrap/templates/gallery.tmpl#L49

@ralsina
Copy link
Member Author

ralsina commented May 14, 2017

I will take another shot at this, but this bitrot way too much.

@ralsina ralsina closed this May 14, 2017
@Kwpolska Kwpolska deleted the refactor-galleries-gain branch May 25, 2017 15:54
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

3 participants