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

Add ThemeAssetsReader which reads assets from a theme #5364

Merged
merged 9 commits into from Sep 20, 2016

Conversation

Projects
None yet
3 participants
@parkr
Member

parkr commented Sep 16, 2016

If the theme includes the 'assets' directory, it will be walked and items will be added to the site based on the normal rules of Jekyll: if there is YAML front matter, it will be added as a (convertible) Page, otherwise it will be added as a StaticFile. It will output to the assets/ directory.

[Image available, but airplane WiFi is spotty.]

  • Add basic functionality
  • Ensure jekyll new-theme looks for assets/ if present
  • Add tests
  • Update the docs

/cc @jekyll/ecosystem @benbalter @broccolini

Add ThemeAssetsReader which reads assets from a theme
If the theme includes the 'assets' directory, it will be walked and items will be added to the site based
on the normal rules of Jekyll: if there is YAML front matter, it will be added as a (convertible) Page,
otherwise it will be added as a StaticFile.

parkr added some commits Sep 16, 2016

@@ -16,7 +16,7 @@ Lint/UnreachableCode:
Lint/UselessAccessModifier:
Enabled: false
Metrics/AbcSize:
Max: 20
Max: 21

This comment has been minimized.

@ghost

ghost Sep 18, 2016

That's cheating! 😉

@ghost

ghost Sep 18, 2016

That's cheating! 😉

This comment has been minimized.

@parkr

parkr Sep 18, 2016

Member

😇

@parkr

parkr Sep 18, 2016

Member

😇

@benbalter

This is awesome. So excited for this. This will hugely simplify the user experience for themes and in improve the developer experience for theme creators. Two other things to do in this PR, in my mind:

  • Update the docs
  • Update the default site to not to write a CSS file (and add that file to minima instead)
@@ -40,7 +40,11 @@ def initialize(site, base, dir, name)
@base = base
@dir = dir
@name = name
@path = site.in_source_dir(base, dir, name)
@path = if site.in_theme_dir(base) == base # we're in a theme

This comment has been minimized.

@benbalter

benbalter Sep 19, 2016

Contributor

Can you explain a bit more as to what's going on here?

@benbalter

benbalter Sep 19, 2016

Contributor

Can you explain a bit more as to what's going on here?

This comment has been minimized.

@ghost

ghost Sep 19, 2016

@benbalter AFAIK, it essentially makes sure that we're normalising into the theme directory, rather than into the source directory, otherwise you might get /home/user/work/home/user/.gems/[...]/theme.html instead of /home/user/.gems/[...]/theme.html.

@parkr Have you tried this out with various symbolic links? I'm worried it might be a bit weird, depending on how base is set (I haven't checked, myself)

@ghost

ghost Sep 19, 2016

@benbalter AFAIK, it essentially makes sure that we're normalising into the theme directory, rather than into the source directory, otherwise you might get /home/user/work/home/user/.gems/[...]/theme.html instead of /home/user/.gems/[...]/theme.html.

@parkr Have you tried this out with various symbolic links? I'm worried it might be a bit weird, depending on how base is set (I haven't checked, myself)

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

@benbalter When you include assets/styles.scss with YAML front matter, it's a Jekyll::Page. To ensure it's read properly, we need to make sure we're looking in the theme's directory rather than your site's directory. This if says "if this Page is inside our theme directory then ensure the @path gets set properly."

@parkr

parkr Sep 19, 2016

Member

@benbalter When you include assets/styles.scss with YAML front matter, it's a Jekyll::Page. To ensure it's read properly, we need to make sure we're looking in the theme's directory rather than your site's directory. This if says "if this Page is inside our theme directory then ensure the @path gets set properly."

module Jekyll
class ThemeAssetsReader
attr_reader :site
def initialize(site)

This comment has been minimized.

@benbalter

benbalter Sep 19, 2016

Contributor

Whitespace here?

@benbalter

benbalter Sep 19, 2016

Contributor

Whitespace here?

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

This is copied from another reader. I don't have a personal preference and we don't have it enforced in our Rubocop config.

@parkr

parkr Sep 19, 2016

Member

This is copied from another reader. I don't have a personal preference and we don't have it enforced in our Rubocop config.

if Utils.has_yaml_header?(path)
append_unless_exists site.pages,
Jekyll::Page.new(site, base, dir, name)
else

This comment has been minimized.

@benbalter

benbalter Sep 19, 2016

Contributor

Really elegant implementation.

@benbalter

benbalter Sep 19, 2016

Contributor

Really elegant implementation.

@@ -43,7 +47,7 @@ def path_for(folder)
end
def realpath_for(folder)
File.realpath(Jekyll.sanitized_path(root, "_#{folder}"))
File.realpath(Jekyll.sanitized_path(root, folder.to_s))

This comment has been minimized.

@benbalter

benbalter Sep 19, 2016

Contributor

I'm torn here. Do we want to pass vanilla symbols (without the _) and special case assets so that we treat all of them as named folders, rather than paths?

@benbalter

benbalter Sep 19, 2016

Contributor

I'm torn here. Do we want to pass vanilla symbols (without the _) and special case assets so that we treat all of them as named folders, rather than paths?

This comment has been minimized.

@ghost

ghost Sep 19, 2016

If we're passing symbols like :_layouts, why is it called realpath_for? It's just like Site#in_theme_dir, except it takes a symbol. I think realpath_for should be an abstraction (special case :assets, for example)

@ghost

ghost Sep 19, 2016

If we're passing symbols like :_layouts, why is it called realpath_for? It's just like Site#in_theme_dir, except it takes a symbol. I think realpath_for should be an abstraction (special case :assets, for example)

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

Special casing is a non-starter for me in this already complicated codebase. I want a way to fetch the path on disk for a directory. If the directory starts with a _, then let's ask for it that way. It's way more direct to ask for _sass and get _sass then to ask for sass and get _sass.

@spudowiar The difference here is that this uses File.realpath which resolves symlinks. Site#in_theme_dir does not.

@parkr

parkr Sep 19, 2016

Member

Special casing is a non-starter for me in this already complicated codebase. I want a way to fetch the path on disk for a directory. If the directory starts with a _, then let's ask for it that way. It's way more direct to ask for _sass and get _sass then to ask for sass and get _sass.

@spudowiar The difference here is that this uses File.realpath which resolves symlinks. Site#in_theme_dir does not.

This comment has been minimized.

@benbalter

benbalter Sep 20, 2016

Contributor

Makes sense. Would a string be a better fit stylistically then?

@benbalter

benbalter Sep 20, 2016

Contributor

Makes sense. Would a string be a better fit stylistically then?

Show outdated Hide outdated test/test_theme_assets_reader.rb
ThemeAssetsReader.new(@site).read
assert_file_with_relative_path @site.static_files, "assets/img/logo.png"
assert_file_with_relative_path @site.pages, "assets/style.scss"
end

This comment has been minimized.

@benbalter

benbalter Sep 19, 2016

Contributor

Do you want to test that it doesn't clobber site files?

@benbalter

benbalter Sep 19, 2016

Contributor

Do you want to test that it doesn't clobber site files?

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

Clobber?

@parkr

parkr Sep 19, 2016

Member

Clobber?

This comment has been minimized.

@benbalter

benbalter Sep 20, 2016

Contributor

If I have /assets/foo.js in my site, and /assets/foo.js in my theme, to confirm /assests/foo.js in the resulting site contain's the site's content, not the theme's.

@benbalter

benbalter Sep 20, 2016

Contributor

If I have /assets/foo.js in my site, and /assets/foo.js in my theme, to confirm /assests/foo.js in the resulting site contain's the site's content, not the theme's.

private
def read_theme_asset(path)
base = site.theme.root
dir = File.dirname(path.sub("#{site.theme.root}/", ""))

This comment has been minimized.

@ghost

ghost Sep 19, 2016

We do this same thing at least 2 different ways elsewhere, in the Jekyll::Layout we use the equivalent of path.sub(site.theme.root, "") and in the Jekyll::Document we use Pathutil and there are probably more ways we're doing this. 😖

@ghost

ghost Sep 19, 2016

We do this same thing at least 2 different ways elsewhere, in the Jekyll::Layout we use the equivalent of path.sub(site.theme.root, "") and in the Jekyll::Document we use Pathutil and there are probably more ways we're doing this. 😖

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

Would be nice to have a "relative_from" method or something but it depends on what you need. In this case, I want all directories within the theme directory without the forward slash at the beginning (e.g. assets). If you do path.sub(site.theme.root, ""), you get /assets and we can't use Pathutil because it doesn't work on Windows the way we're using it.

@parkr

parkr Sep 19, 2016

Member

Would be nice to have a "relative_from" method or something but it depends on what you need. In this case, I want all directories within the theme directory without the forward slash at the beginning (e.g. assets). If you do path.sub(site.theme.root, ""), you get /assets and we can't use Pathutil because it doesn't work on Windows the way we're using it.

@@ -56,7 +56,7 @@ def setup
should "return the resolved path when a symlink & resolved path exists" do
expected = File.expand_path("./_layouts", @expected_root)
assert_equal expected, @theme.send(:path_for, :symlink)

This comment has been minimized.

@ghost

ghost Sep 19, 2016

Not happy with this at all, Theme#path_for sounds like it should be an abstraction, looks like it's just a sugary version of Site#in_theme_dir

@ghost

ghost Sep 19, 2016

Not happy with this at all, Theme#path_for sounds like it should be an abstraction, looks like it's just a sugary version of Site#in_theme_dir

This comment has been minimized.

@parkr

parkr Sep 19, 2016

Member

It's not, as I mention above.

@parkr

parkr Sep 19, 2016

Member

It's not, as I mention above.

parkr added some commits Sep 19, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 19, 2016

Member

@benbalter I think we're ready for another review. I left a few comments. As for "Update the default site to not to write a CSS file (and add that file to minima instead)", this will have to be in another PR which results from a minima release. I don't think that's a good idea to do here.

Member

parkr commented Sep 19, 2016

@benbalter I think we're ready for another review. I left a few comments. As for "Update the default site to not to write a CSS file (and add that file to minima instead)", this will have to be in another PR which results from a minima release. I don't think that's a good idea to do here.

@benbalter

One question about if we need a test to ensure the files in the site's assets folder are given preference over files in the theme's asset folder, otherwise, LGTM. Nice work. Glad this wasn't too crazy to implement.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 20, 2016

Member

One question about if we need a test to ensure the files in the site's assets folder are given preference over files in the theme's asset folder

Done in 29d8fee.

Member

parkr commented Sep 20, 2016

One question about if we need a test to ensure the files in the site's assets folder are given preference over files in the theme's asset folder

Done in 29d8fee.

Merge branch 'master' into themes-asset-folder
* master:
  Update history to reflect merge of #5381 [ci skip]
  Update history to reflect merge of #5383 [ci skip]
  run features on windows
  Appease Rubocop
  Update history to reflect merge of #5372 [ci skip]
  Add missing period to sentence in first paragraph.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 20, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Sep 20, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 3a45bf7 into master Sep 20, 2016

@jekyllbot jekyllbot deleted the themes-asset-folder branch Sep 20, 2016

@parkr parkr added this to the 3.3 milestone Sep 20, 2016

jekyllbot added a commit that referenced this pull request Sep 20, 2016

@mmistakes mmistakes referenced this pull request Sep 22, 2016

Closed

Convert theme into a Ruby Gem #419

10 of 11 tasks complete

@benbalter benbalter referenced this pull request Dec 22, 2016

Closed

Theme assets do not respect static files within the site #5676

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment