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

Ditch google cdn and add swiping to colorbox theme #116

Closed
wants to merge 2 commits into from

Conversation

@t-animal
Copy link
Contributor

@t-animal t-animal commented Oct 3, 2014

I did these mainly for my own installation, but I think these can be useful for the upstream version, too:

bdef57a makes serving of the css and js files by google optional, increasing security and privacy (plus this has benefits for galleries, which are not served by an webserver)
c70900c adds swiping to the colorbox theme (colorbox does not support this out of the box), which is handy if you're watching the images on your mobile device

@saimn
Copy link
Owner

@saimn saimn commented Oct 3, 2014

Hi @t-animal !
Giving the choice to use google stuff is instead a nice idea, I'm all in favor of it. The issue I see is about license compatibility: what is the impact of the SIL Open Font License ?
For the swipe plugin, I don't know if it is possible to detect touch event support before activating the plugin ?

@t-animal t-animal force-pushed the t-animal:master branch from c70900c to 5d228a5 Oct 4, 2014
@t-animal
Copy link
Contributor Author

@t-animal t-animal commented Oct 4, 2014

IANAL, but wikipedia says "The Open Font License is a free software license, and as such permits the fonts to be used, modified, and distributed freely (so long as the resulting fonts remain under the Open Font License)", which sounds like it's ok to use it and according to this you don't have to mention the original author.

For the touch event support, to be honest, I don't know. I looked up whether colorbox can do swiping out of the box and in the discussion of the corresponding issue found a reference to touchswipe on which I build this. It doesn't mess with desktop systems, though, so I don't think it's necessary to detect if it's run on a mobile system. And as I don't see it in the touchswipe documentation, I believe it's not worth the effort.

@saimn
Copy link
Owner

@saimn saimn commented Oct 20, 2014

Hi @t-animal ,
ok for the licenses ! I have tested your code, but the fonts does not load on Firefox (it works only with Chrome).
PT Sans can be downloaded here: http://www.paratype.com/public/ and the PTS55F_* files tell how to use the font. For Molengo the source seems to be here: https://code.google.com/p/molengo-fonts/
Could you update your PR to use this ?

@@ -105,6 +105,9 @@
# Google Analytics tracking code (UA-xxxx-x)
# google_analytics = ''

# Use google CDN to serve some css and js files or serve them yourself

This comment has been minimized.

@saimn

saimn Oct 20, 2014
Owner

Mention fonts in the comments ?

saimn added a commit that referenced this pull request Aug 28, 2015
@saimn
Copy link
Owner

@saimn saimn commented Aug 28, 2015

Hi @t-animal
I have finally merged: I merged the swipe commit manually, and for the cdn setting I choose to not replace the fonts with local ones, it is easier to do and it avoids to increase the size of the zip archive, so it seems a good option.
Thanks for your work on this.

@saimn saimn closed this Aug 28, 2015
@ghost
Copy link

@ghost ghost commented Aug 30, 2015

I would prefer not to load fonts from google but provide them locally.

@saimn
Copy link
Owner

@saimn saimn commented Aug 30, 2015

Yes, it would be better. The current patch is a first step. Then there is some work to check all the fonts needed (and include only the fonts actually used to minimize the size), the formats to work on all browsers, etc. Would you like to help on this ?

@saimn saimn modified the milestone: 0.10 Aug 31, 2015
@ghost
Copy link

@ghost ghost commented Sep 2, 2015

kontza pushed a commit to kontza/sigal that referenced this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.