fixes & improvements #11
Conversation
Would you please add that case to tests ? Because it seems existing test cases passing a variable work properly. |
|
Hi, I did update test case you've had ('spec/jekyll-linkpreview_spec.rb') to reflect changes I made. Unfortunately, I do not have time to create new test cases now - however, if you wish to verify, it's enough to create new Jekyll site (e.g. jekyll new jekyll-linkpreview-testsite), add the plugin (currently released version, i.e. 0.2.0), then attempt to use variables instead of strings, and you'll see how this works, i.e. it does not. Also, you have an existing open issue #2 which talks about the same thing, i.e. not being able to use variables in Jekyll sites when using current version of your plugin. So, to summarize, having this work in an actual deployment (i.e. Jekyll site) is more important than automated test cases which obviously are missing something because the two are not in sync. Hope this helps. Michael |
|
It seems jekyll-linkpreview v0.2.0 does work with variables. and confirmed it works fine like this.
(the image is so big because no css is applied ...) I want to know why your attempt to use variables fails. That's why I want you to add a testcase, which fails with v0.2.0 and passes with this pull request. |
| 1. Place `linkpreview_nog.html` file inside `_includes/` folder of your Jekyll site (`_includes/linkpreview_nog.html`) | ||
|
|
||
| 2. Use built-in **link_url** variable to render URL data, i.e. `{{ link_url }}` | ||
|
|
There was a problem hiding this comment.
Thank you for the detailed explanation!
| @@ -122,7 +96,7 @@ def get_properties(url) | |||
|
|
|||
| private | |||
| def get_url_from(context) | |||
| context.scopes[0].key?(@markup) ? context.scopes[0][@markup] : @markup | |||
| context[@markup] | |||
There was a problem hiding this comment.
Becomes much simpler!
Now users have to pass quoted url like {% linkpreview 'https://github.com' %}, but it may be acceptable.
| @@ -132,7 +106,69 @@ def load_cache_file(filepath) | |||
|
|
|||
| protected | |||
| def save_cache_file(filepath, properties) | |||
| File.open(filepath, 'w').write(JSON.generate(properties)) | |||
| File.open(filepath, 'w') { |f| f.write JSON.generate(properties) } | |||
| <a href="//#{domain}" target="_blank">#{domain}</a> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Indentation should be deleted because it doesn't rendered properly.
| html = <<-EOS | ||
| <div class="jekyll-linkpreview-wrapper"> | ||
| <p><a href="#{url}" target="_blank">#{url}</a></p> | ||
| </div> |
There was a problem hiding this comment.
Indentation should be deleted, same reason above.
|
There is a problem (heredoc indent) in this PR, but I'll fix it after merge. |
|
Hi, Sorry I could not help with formatting, I did not have the time to check GitHub since then. Thank you for the merge - glad I could help. |
Hi,
I just committed few fixes and improvements which I needed to make. Those are:
FIX: file not being closed
In
lib/jekyll-linkpreview.rb, functionsave_cache_filehad a problem due to file not being closed. This error could be seen on first generation / when no cache entry existed for given URL and attempt to create then read cache entry for given URL was made.FIX: variable not being extracted properly
This error prevented me from passing URLs as variables to the linkpreview tag. Now either string or a variable can be passed with no problems.
IMPROVEMENT: ability to customize layout via template - instead of using those embedded in plugin source code
More information about this in code and the updated README.md file.
Other:
Some of these fixes and improvements actually coinicide with issues I noticed you had open, which you can now close, i.e.:
Regards,
Michael