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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a new `relative_include` tag #2870

Merged
merged 10 commits into from Sep 7, 2014

Conversation

@gjtorikian
Copy link
Member

@gjtorikian gjtorikian commented Sep 3, 2014

This PR aims to solve a problem reported here and elsewhere.

When it comes to assets, I'm perfectly happy with the following import strategy:


---

---

{% include assets/javascript/jquery.min.js %}
{% include assets/javascript/foo.js %}
{% include assets/javascript/whatever.js %}

What bothers me, though, is that I must place my assets in the _includes folder. Given that SASS has its own import notation, and that I want to be a good citizen and use Bower for imports, it becomes impossible to keep all my assets tidy and in a single location.

With relative_include, I should be able to include files relative to the file they're defined in. I should not be able to do ../ and go somewhere else--that's bad. Most of this checking--along with evil symlinks--is already accomplished by the current IncludeTag, which is very nice.

Please let me know how you feel about this. 馃挓

@parkr
Copy link
Member

@parkr parkr commented Sep 4, 2014

I like it. Feels like it overlaps with collections, though... Collection documents are meant to be pieces of include-able stuff. That's why collections are optionally rendered. Have you tried this with collections?

@gjtorikian
Copy link
Member Author

@gjtorikian gjtorikian commented Sep 4, 2014

I've just tried working it out in collections, here's my experience so far.

I create an _assets directory to start with. As I would like to keep my SASS and JS/CS in separate, but within one directory, I create two subfolders for each of them.

As you pointed out, I can flip output to false to not have these files in the rendered output. Now, to collect the content, I can do something like the following in a layout:

{% for asset in site.assets %}
  {{ asset.output }}
{% endfor %}

And here's the rub (as I see it): we're just dumping the contents straight into the HTML. We're not able to compress it, or even generate a single file. Well, that's not entirely true: I can create a separate file outside of my _assets directory, whose sole purpose it is to collect the CSS and JS within the loop, so that it can be included in the project. Now I have two assets directories.

I can push to a dummy repo to show what I mean, but I'm trying to work out a way for there to be just a single assets directory that contains all the assets. For Sass, this is pretty easy. Create assets/stylesheets, then create a file called application.scss, with these contents:

---
---
@import 'globals';
@import 'plugins';
@import 'sections';
@import 'shared';

In _config.yml, write the following and you're done:

sass:
    sass_dir: assets/stylesheets/
    style: :compressed

For JS, you can create a single file like the one above. And with relative_include, you can create subfiles within the same directory. And all of it would be output as a single JS file.

Am I mistaken somewhere?

@parkr parkr mentioned this pull request Sep 5, 2014
0 of 2 tasks complete
@@ -138,7 +148,11 @@ def source(file, context)
File.read(file, file_read_opts(context))
end
end

class RelativeIncludeTag < IncludeTag
end

This comment has been minimized.

@parkr

parkr Sep 5, 2014
Member

Why use case statements instead of just overwrite certain methods based on the tag class? If you have a separate class, you may as well separate the class-specific code from each other. 馃槈

This comment has been minimized.

@gjtorikian

gjtorikian Sep 5, 2014
Author Member

true.dat

This comment has been minimized.

@parkr

parkr Sep 5, 2014
Member

If you want me to do that, I am happy to submit a PR to your PR. Or something? #howtogithub

This comment has been minimized.

@gjtorikian

gjtorikian Sep 5, 2014
Author Member

Well, I was planing on just killing this and relying more on tag_name to do the weight. There's only two spots that need to change. Does that make sense? #howdoiprogram

@doktorbro
Copy link
Member

@doktorbro doktorbro commented Sep 7, 2014

Please note, that all includes run through the Liquid parser. I remember issues in the past with including sass files, because Liquid fails on invalid {% abc.

Is include_relative a stupid concatenation or is the Liquid parser involved?

def render(context)
dir = File.join(File.realpath(context.registers[:site].source), INCLUDES_DIR)
dir = dir_to_include(context)

This comment has been minimized.

@parkr

parkr Sep 7, 2014
Member

dir_to_include? Are we including the whole dir? Or is it resolved_includes_dir?

This comment has been minimized.

@gjtorikian

gjtorikian Sep 7, 2014
Author Member

The second one. 馃槉


def dir_to_include(context)
page_path = context.registers[:page].nil? ? includes_dir : File.dirname(context.registers[:page]["path"])
File.join(File.realpath(context.registers[:site].source), page_path)

This comment has been minimized.

@parkr

parkr Sep 7, 2014
Member

We should ensure this path is sanitized properly. Would you please change this line to

Jekyll.sanitized_path(context.registers[:site].source, page_path)

? The realpath stuff is resolved later on in the including process so we don't need that here.

@parkr
Copy link
Member

@parkr parkr commented Sep 7, 2014

Lookin' preeeeety good! Just a couple comments above. 鉂わ笍

parkr added a commit that referenced this pull request Sep 7, 2014
@parkr parkr merged commit 6644c77 into jekyll:master Sep 7, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
parkr added a commit that referenced this pull request Sep 7, 2014
@parkr
Copy link
Member

@parkr parkr commented Sep 7, 2014

MERGED. 馃幆

Would you mind writing some docs for this? There are docs already for the include tag 鈥 we should be able to latch onto those.

@gjtorikian
Copy link
Member Author

@gjtorikian gjtorikian commented Sep 7, 2014

@parkr More than these? gjtorikian@afd30b0

@gjtorikian gjtorikian deleted the gjtorikian:support-relative-include branch Sep 7, 2014
@parkr
Copy link
Member

@parkr parkr commented Sep 7, 2014

Yeah -- there will be confusion (probably) around how relativity is applied there. Relative to site source? To includes dir? A bit more verbiage might halp :)

@brianzelip
Copy link

@brianzelip brianzelip commented Sep 16, 2014

yes please more verbiage. i'm studying this right now (via github.com/jekyll/site/_includes/docs_ul.html):

{% assign items = include.items %}

<ul>
{% for item in items %}
  {% assign item_url = item | prepend:"/docs/" | append:"/" %}

  {% if item_url == page.url %}
    {% assign c = "current" %}
  {% else %}
    {% assign c = "" %}
  {% endif %}

  {% for p in site.docs %}
    {% if p.url == item_url %}
      <li class="{{ c }}"><a href="{{ site.url }}{{ p.url }}">{{ p.title }}</a></li>
    {% endif %}
  {% endfor %}

{% endfor %}
</ul>

I figure it's got something to do with this PR, but still don't get it.

@parkr
Copy link
Member

@parkr parkr commented Sep 16, 2014

The above code has nothing to do with this PR.

@mhulse
Copy link

@mhulse mhulse commented Oct 4, 2014

+1 for relative includes! Any idea when this will make it into the gh-pages gem?

@gjtorikian
Copy link
Member Author

@gjtorikian gjtorikian commented Oct 4, 2014

Any idea when this will make it into the gh-pages gem?

It already is! We shipped Jekyll 2.4.0, which includes this, a few weeks ago.

@mhulse
Copy link

@mhulse mhulse commented Oct 4, 2014

It already is! We shipped Jekyll 2.4.0, which includes this, a few weeks ago.

OMG, nice! 馃憤 :octocat:

That just made my weekend. Thanks @gjtorikian!!!

@ryanburnette
Copy link
Contributor

@ryanburnette ryanburnette commented Jan 24, 2015

Are there any docs for this?

@ryanburnette
Copy link
Contributor

@ryanburnette ryanburnette commented Jan 24, 2015

Thanks!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can鈥檛 perform that action at this time.