Added an noscript fallback for browsers without javascript enabled. #7

gr4y · 2015-05-08T14:46:23Z

This would be better for RSS readers and users with tinfoil hats and disabled javascript. It fetches the code via HTTP, escapes it and wraps into an noscript-Tag. To make the specs work again I had to add webmock to the development dependencies, so I had to update all specs and added new it-blocks to each context and an before-Block to stub the request. 💪

If there are any questions, don't hesitate to answer. I hope dearly that you will accept the PR, so all jekyll-gist users can profit from that change soon. 😄

Have a great day 🌞

parkr · 2015-05-11T01:24:52Z

This is excellent!!!

parkr · 2015-05-11T01:25:48Z

lib/jekyll-gist/gist_tag.rb

+        else
+          uri = "https://gist.githubusercontent.com/#{gist_id}/raw/#{filename}"
+        end
+        code = open(uri).read.chomp


What happens if I'm offline? (Or if I don't get a 200)

Thanks for pointing that out, I will fix that in the next couple of days.

Ok! This is a great improvement and I'm excited to have it. Let me know if you need anything from me to get this done. 😃

…e script-Tag

gr4y · 2015-05-12T17:18:37Z

So I added error handling. The user won't notice it when the raw code could not be fetched and the noscript-Tag will not be rendered in that case. I thought about even not rendering the script-Tag but I guess that would be kinda stupid, cause it might be possible that raw.githubusercontent.com is down and the javascript-Service isn't. 😄

parkr · 2015-05-12T17:37:00Z

Truth! Thoughts on a warning message?

gr4y · 2015-05-12T18:06:33Z

I am conflicted about a warning, cause it could confuse users and I don't rank the importance of that noscript tag that high that it deserves an own warning if you know what I mean. On the other hand I'd like to be warned when my site is build incompletely. (I wouldn't have implemented that feature if it wouldn't have been important to me to have the noscript tag in my rss feed.)

So I am opting in on an warning message shown in advanced user mode. 😉

parkr · 2015-05-12T18:07:52Z

As long as the warning is crafted as such and doesn't change the exit code of the process, I think most users will be fine with it and will understand that it's not going to ruin their site. And hopefully they will be able to see that too.

Jekyll.logger.warn "Warning:", "The <noscript> tag for your gist #{gist_id} could not be generated. This will affect users who do not have JavaScript available or enabled in their browsers."

?

gr4y · 2015-05-12T18:27:52Z

So what's your suggestion for that message? I am not that good at writing warning messages, I guess.
"Could not fetch raw code of gist #{gist_id}" sounds a little bland.

parkr · 2015-05-12T22:26:17Z

@gr4y See my comment.

…d when the response is an 404 an HTTPError will be raised. Rescue both of them now.

…hing. ;)

gr4y · 2015-06-03T13:04:23Z

Hi @parkr,

I just want to check if I can help you with anything to get that PR merged. Please drop me a line if there's anything that I can help you with regarding my code. 😄

gr4y · 2015-06-10T17:09:27Z

I am so sorry, but I completely forgot about that until now. I just pushed the change into my fork. 😁

parkr · 2015-06-10T21:01:46Z

lib/jekyll-gist/gist_tag.rb

+          uri = "https://gist.githubusercontent.com/#{gist_id}/raw"
+        else
+          uri = "https://gist.githubusercontent.com/#{gist_id}/raw/#{filename}"
+        end


What do you think about

uri = "https://gist.githubusercontent.com/#{gist_id}/raw" url = "#{uri}/#{filename}" unless filename.empty?

to replace the if here?

I think it does look cleaner.
I guess the same could be done for the gist_script_tag method:

def gist_script_tag(gist_id, filename = nil) url = "https://gist.github.com/#{gist_id}.js" url = "#{url}?file=#{filename}" unless filename.empty? "<script src=\"#{url}\"> </script>" end

parkr · 2015-07-26T00:20:07Z

After a little while, I'm still not fully convinced on this one. I like the idea in theory, but if we have to download the raw content, why not just insert that?

gr4y · 2015-07-26T19:01:23Z

Don't get me wrong, but when one has to insert the raw content into the document one doesn't need the gist tag at all.

It was absolutely frustrating for me to see posts in my rss reader where people talk about a code snippet and I couldn't see that code snippet without opening the post in my mobile browser. And I didn't want my readers to feel the same frustration I feel when I read posts in my rss reader. Sure I could have put the code snippets into the document itself, but what if I change the snippets later? With downloading the raw content from gist.github.com it will even update the code people see in rss readers or browsers with disabled javascript when I rebuild the page.

pathawks · 2015-07-26T19:32:41Z

With downloading the raw content from gist.github.com it will even update the code people see in rss readers or browsers with disabled javascript when I rebuild the page.

👍

It’s too bad that the Gist embed code on GitHub’s end isn’t already built with progressive enhancement in mind.

gr4y · 2015-08-03T18:10:15Z

So I just wanted to read something in Pocket and I clicked on the post I wanted to read, scrolled down to the point where the code should be... And to my surprise the code snippets are completely missing.

The Blog is hosted on GitHub Pages which still is using Jekyll 1, i assume? URL to the Post: http://nesv.github.io/golang/2014/02/25/worker-queues-in-go.html

parkr · 2015-08-03T19:40:59Z

lib/jekyll-gist/gist_tag.rb

+        begin
+          url = "https://gist.githubusercontent.com/#{gist_id}/raw"
+          url = "#{url}/#{filename}" unless filename.empty?
+          open(url).read.chomp


Should we use .strip instead of .chomp?

parkr · 2015-08-03T19:41:15Z

Ready to merge pending that one question above.

Merge pull request 7

[ci skip]

parkr · 2015-08-05T22:28:55Z

As someone who now surfs the web without JavaScript enabled, this will be great to have. Thanks, @gr4y!

parkr · 2015-08-05T22:33:31Z

https://github.com/jekyll/jekyll-gist/releases/tag/v1.3.0

https://twitter.com/jekyllrb/status/629057572440403968

raelgc · 2015-08-06T11:54:37Z

The side effect: today I've updated my gems, and now any simple change in my local is generating a 30 seconds delay, due the all gist usage in the site generating warnings.

Warning: The <noscript> tag for your gist xxxxx.

parkr · 2015-08-06T15:02:30Z

Is it timing out? Can you paste the full error with --trace?

gr4y · 2015-08-06T15:33:42Z

Well... I thought about that some time ago and I know that it can be a problem on really bad internet connections and I came up with several ideas to solve that.

Setting the timeout to 10 seconds, which should be quite okay for most of the people, but currently my internet connection is flaky from time to time, so it would result in generated posts with noscript fallback code and some without, which is annoying too.

I even added some caching to the tag back in the days when I implemented that in my own fork and that might be the way to go, but I didn't like the thought of a changing the gists and upon the next build of the site the updated code won't be fetched because it is cached, which results in the gist and the actual noscript fallback code in the post diverging.

I would love to hear your thoughts on that. I may take some time to respond, though.

parkr · 2015-08-06T16:16:10Z

Making this optional might work. Could do that and enterprise host support in a 1.4.0

gr4y · 2015-08-06T17:06:37Z

I thought about that too, but I am not a huge fan of adding more switches and toggles to any project. Being able to disable the feature would be okay, but I don't like it to be disabled by default. I don't know who to contact at GitHub to make them think about adding progressive enhancement to the gist embed code, but I think that should be the long term solution for everyone.

parkr · 2015-08-06T18:09:48Z

I don't know who to contact at GitHub

Write support@github.com. They'll give you a canned response but it'll be on their radar then.

I agree that we want this. I think a timeout of like 3 seconds is more than enough. 3 seconds for an API call is pretty absurd by any standard. If it is dropped, we should retry.

gr4y · 2015-08-06T20:15:26Z

I just looked into timeouts in OpenURI and found out that there is an read_timeout option. With Ruby 2.2 there is another open_timeout option, which is exactly the one we might need. But just passing it into OpenURI causes an exception, telling me that the option is not valid. (Geez, that smeels like some Java developers started to do Ruby... 😖) 😁

Maybe using NET::HTTP instead of OpenURI will do the trick, but my concentration is just fading away, so I will take another look at that later, when I am not tired.

parkr · 2015-08-06T22:09:02Z

Maybe using NET::HTTP instead of OpenURI will do the trick

Definitely a Good Idea™. Ready to review a PR when you're rested!

parkr · 2015-08-16T17:47:38Z

@raelgc Does v1.3.1 help?

raelgc · 2015-08-19T11:34:23Z

@parkr Thank you for the update. I'm just waiting to get it via bundle update. As soon as I get it, I'll update here :)

parkr · 2015-08-19T15:12:20Z

@parkr Thank you for the update. I'm just waiting to get it via bundle update. As soon as I get it, I'll update here :)

@raelgc We had to yank it because of #13. Fixing in #14 if you can help.

parkr · 2015-08-19T21:04:51Z

@raelgc Try 1.3.2

Added an noscript fallback for browsers without javascript enabled.

4e7d870

parkr reviewed May 11, 2015
View reviewed changes

If an HTTPError occures the noscript-Tag will not be rendered, but th…

56aef57

…e script-Tag

gr4y added 4 commits May 13, 2015 06:08

Turns out open-uri raises an SocketError and no HTTPError

4a24dcd

Added warning when <noscript> tag could not be generated

93e91fa

So it turns out when you are offline an SocketError will be raised an…

a5d65cf

…d when the response is an 404 an HTTPError will be raised. Rescue both of them now.

I had to fix the language of that one example, cause me eye was twitc…

4ca4831

…hing. ;)

warning now are three warnings with around 55 chars per line

748c701

parkr reviewed Jun 10, 2015
View reviewed changes

gr4y added 2 commits June 11, 2015 08:51

cleaned up fetch_raw_code method

6ea01cf

cleaned up gist_script_tag method

13e6357

parkr reviewed Aug 3, 2015
View reviewed changes

parkr added a commit that referenced this pull request Aug 5, 2015

Merge pull request #7 from gr4y/master

730e45b

Merge pull request 7

parkr merged commit 730e45b into jekyll:master Aug 5, 2015

parkr added a commit that referenced this pull request Aug 5, 2015

Update history to reflect merge of #7

d032487

[ci skip]

gr4y mentioned this pull request Aug 7, 2015

Replaced OpenURI with NET::HTTP and introduced timeout of 3 seconds #11

Merged

parkr mentioned this pull request Aug 18, 2015

Re-add support for Ruby 1.9.3 #14

Merged

tiagoamaro mentioned this pull request Aug 22, 2015

noscript fallback solution slows down site generation #17

Closed

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added an noscript fallback for browsers without javascript enabled. #7

Added an noscript fallback for browsers without javascript enabled. #7

gr4y commented May 8, 2015

parkr commented May 11, 2015

parkr May 11, 2015

gr4y May 11, 2015

parkr May 11, 2015

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented Jun 3, 2015

gr4y commented Jun 10, 2015

parkr Jun 10, 2015

gr4y Jun 11, 2015

parkr commented Jul 26, 2015

gr4y commented Jul 26, 2015

pathawks commented Jul 26, 2015

gr4y commented Aug 3, 2015

parkr Aug 3, 2015

parkr commented Aug 3, 2015

parkr commented Aug 5, 2015

parkr commented Aug 5, 2015

raelgc commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

parkr commented Aug 16, 2015

raelgc commented Aug 19, 2015

parkr commented Aug 19, 2015

parkr commented Aug 19, 2015

Added an noscript fallback for browsers without javascript enabled. #7

Added an noscript fallback for browsers without javascript enabled. #7

Conversation

gr4y commented May 8, 2015

parkr commented May 11, 2015

parkr May 11, 2015

Choose a reason for hiding this comment

gr4y May 11, 2015

Choose a reason for hiding this comment

parkr May 11, 2015

Choose a reason for hiding this comment

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented May 12, 2015

parkr commented May 12, 2015

gr4y commented Jun 3, 2015

gr4y commented Jun 10, 2015

parkr Jun 10, 2015

Choose a reason for hiding this comment

gr4y Jun 11, 2015

Choose a reason for hiding this comment

parkr commented Jul 26, 2015

gr4y commented Jul 26, 2015

pathawks commented Jul 26, 2015

gr4y commented Aug 3, 2015

parkr Aug 3, 2015

Choose a reason for hiding this comment

parkr commented Aug 3, 2015

parkr commented Aug 5, 2015

parkr commented Aug 5, 2015

raelgc commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

gr4y commented Aug 6, 2015

parkr commented Aug 6, 2015

parkr commented Aug 16, 2015

raelgc commented Aug 19, 2015

parkr commented Aug 19, 2015

parkr commented Aug 19, 2015