Gem-based themes #4595

Merged
merged 40 commits into from Apr 22, 2016

Conversation

Projects
None yet
6 participants
@benbalter
Contributor

benbalter commented Feb 26, 2016

This is a very early attempt to try to figure out what would be involved to support #4510.

Right now, adding theme: foo to your config file, will init the foo theme, and the theme can find itself on the filesystem, but that's about it.

@benbalter benbalter self-assigned this Feb 26, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 29, 2016

Ooh, just noticed this when I'd already started working on this! My method turned layouts_dir into an array or a string, the array case obviously using multiple layout directories. Rinse and repeat for other types of directory.

ghost commented Feb 29, 2016

Ooh, just noticed this when I'd already started working on this! My method turned layouts_dir into an array or a string, the array case obviously using multiple layout directories. Rinse and repeat for other types of directory.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Feb 29, 2016

Contributor

@spudowiar that's awesome. Ignoring the change to lib/jekyll/readers/layout_reader.rb (which didn't work), it sounds like we could combine our efforts, by doing something like layouts_dir << site.theme.layouts_path (and rinse and repeat for other directories)?

Contributor

benbalter commented Feb 29, 2016

@spudowiar that's awesome. Ignoring the change to lib/jekyll/readers/layout_reader.rb (which didn't work), it sounds like we could combine our efforts, by doing something like layouts_dir << site.theme.layouts_path (and rinse and repeat for other directories)?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 29, 2016

That's what I'm hoping. Patch should be complete soon. AFAICT the only other directories are includes_dir and sass_dir in jekyll-sass-converter

ghost commented Feb 29, 2016

That's what I'm hoping. Patch should be complete soon. AFAICT the only other directories are includes_dir and sass_dir in jekyll-sass-converter

@ghost ghost referenced this pull request Feb 29, 2016

Closed

Allow multiple directories #4610

@benbalter benbalter changed the title from WIP: Gem-based themes to Gem-based themes Mar 5, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 5, 2016

Contributor

Pushed up passing tests. Believe this should solidly create a Theme class, that can safely determine its own paths. Along with @spudowiar work over in #4610 to support multiple paths, it looks like the basic implementation of #4510 is coming together.

Contributor

benbalter commented Mar 5, 2016

Pushed up passing tests. Believe this should solidly create a Theme class, that can safely determine its own paths. Along with @spudowiar work over in #4610 to support multiple paths, it looks like the basic implementation of #4510 is coming together.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 6, 2016

Contributor

Pushed up two more rounds of changes:

  1. Added documentation for where I'm hoping things will land.
  2. Moved theme assets from /_assets to /assets. It makes the code slightly more complex, but would allow theme developers to preview their theme by simply using jekyll build and jekyll serve within the theme folder, without having to do any /_assets -> /assets rewriting magic.
Contributor

benbalter commented Mar 6, 2016

Pushed up two more rounds of changes:

  1. Added documentation for where I'm hoping things will land.
  2. Moved theme assets from /_assets to /assets. It makes the code slightly more complex, but would allow theme developers to preview their theme by simply using jekyll build and jekyll serve within the theme folder, without having to do any /_assets -> /assets rewriting magic.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 6, 2016

Contributor

@benbalter I don't see why that is necessary, do Jekyll collections not work with static files?

Contributor

envygeeks commented Mar 6, 2016

@benbalter I don't see why that is necessary, do Jekyll collections not work with static files?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 6, 2016

Contributor

@benbalter @parkr Actually, that might be a good idea, create a default collection and have it output, instead of doing all that, then Jekyll-Assets can come in and remove that collection if the user chooses to use us and process independent of that. This makes it so that asset authors can get the benefit of all worlds no?

Contributor

envygeeks commented Mar 6, 2016

@benbalter @parkr Actually, that might be a good idea, create a default collection and have it output, instead of doing all that, then Jekyll-Assets can come in and remove that collection if the user chooses to use us and process independent of that. This makes it so that asset authors can get the benefit of all worlds no?

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 7, 2016

Contributor

This makes it so that asset authors can get the benefit of all worlds no?

@envygeeks I'm not sure I follow. How would you collections to implement what's described in #4510? The idea would be to move presentation assets out of the repository, and someplace where they can be commonly distributed, versioned, and benefit from upstream changes.

The motivation here is less so "I want a better way to organize the assets that are specific to my site" and more so "I want a better way to use an open source theme somebody else created".

Contributor

benbalter commented Mar 7, 2016

This makes it so that asset authors can get the benefit of all worlds no?

@envygeeks I'm not sure I follow. How would you collections to implement what's described in #4510? The idea would be to move presentation assets out of the repository, and someplace where they can be commonly distributed, versioned, and benefit from upstream changes.

The motivation here is less so "I want a better way to organize the assets that are specific to my site" and more so "I want a better way to use an open source theme somebody else created".

History.markdown
@@ -1,3 +1,5 @@
+# History
+

This comment has been minimized.

@parkr

parkr Mar 8, 2016

Member

Please remove 😄

@parkr

parkr Mar 8, 2016

Member

Please remove 😄

lib/jekyll/theme.rb
+
+ def initialize(name)
+ @name = name.downcase.strip
+ raise Jekyll::Errors::MissingDependencyException unless gemspec

This comment has been minimized.

@parkr

parkr Mar 8, 2016

Member

Would you please add a message to this raise as a second argument?

unless gemspec
  raise Jekyll::Errors::MissingDependencyException, "#{name} could not be found on your system."
end

Also, what motivated your choice to place this logic here? Just an early fail?

@parkr

parkr Mar 8, 2016

Member

Would you please add a message to this raise as a second argument?

unless gemspec
  raise Jekyll::Errors::MissingDependencyException, "#{name} could not be found on your system."
end

Also, what motivated your choice to place this logic here? Just an early fail?

lib/jekyll/theme.rb
+ end
+
+ def version
+ gemspec.version

This comment has been minimized.

@parkr

parkr Mar 8, 2016

Member

Please use a def_delegator (via Forwardable) for this.

@parkr

parkr Mar 8, 2016

Member

Please use a def_delegator (via Forwardable) for this.

site/_config.yml
@@ -2,6 +2,9 @@ markdown: kramdown
highlighter: pygments
permalink: /news/:year/:month/:day/:title/
excerpt_separator: ""
+kramdown:
+ input: GFM
+ hard_wrap: false

This comment has been minimized.

@parkr

parkr Mar 8, 2016

Member

I believe this has deleterious effects on the markup of the site. Please submit this in another pull request.

@parkr

parkr Mar 8, 2016

Member

I believe this has deleterious effects on the markup of the site. Please submit this in another pull request.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 8, 2016

Member

This is great work, @benbalter! Thank you for this pull request. Some concerns above.

How do you plan on integrating the theme into the render process?

Member

parkr commented Mar 8, 2016

This is great work, @benbalter! Thank you for this pull request. Some concerns above.

How do you plan on integrating the theme into the render process?

@parkr parkr added the feature label Mar 8, 2016

@parkr parkr added this to the 3.2 milestone Mar 8, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 8, 2016

Contributor

@parkr believe I addressed your feedback.

How do you plan on integrating the theme into the render process?

Once @spudowiar's #4610 lands, includes, layouts and sass should be as simple as:

site.includes_dir << theme.includes_path
site.layouts_dir  << theme.layouts_path
site.sass.source  << theme.sass_path

I'm waiting to see what happens in #4610 to implement that.

The last remaining step would be assets. Any thoughts on the best way to do that?

Contributor

benbalter commented Mar 8, 2016

@parkr believe I addressed your feedback.

How do you plan on integrating the theme into the render process?

Once @spudowiar's #4610 lands, includes, layouts and sass should be as simple as:

site.includes_dir << theme.includes_path
site.layouts_dir  << theme.layouts_path
site.sass.source  << theme.sass_path

I'm waiting to see what happens in #4610 to implement that.

The last remaining step would be assets. Any thoughts on the best way to do that?

site/_docs/history.md
@@ -1,5 +1,5 @@
---
-title: History
+title:

This comment has been minimized.

@parkr

parkr Mar 9, 2016

Member

Ruh roh

@parkr

parkr Mar 9, 2016

Member

Ruh roh

site/_docs/themes.md
+* `/_layouts`
+* `/_includes`
+* `/_sass`
+* `/assets/`

This comment has been minimized.

@parkr

parkr Mar 9, 2016

Member

does this need the trailing slash?

@parkr

parkr Mar 9, 2016

Member

does this need the trailing slash?

site/_docs/themes.md
+ s.license = 'MIT'
+ s.summary = 'This is an awesome Jekyll theme!'
+ s.author = 'Dr. Jekyll'
+ s.email = 'doc@jekyllrb.com'

This comment has been minimized.

@parkr

parkr Mar 9, 2016

Member

we should probably make this blank or invalid so folks don't release with these values (inevitably someone will)

@parkr

parkr Mar 9, 2016

Member

we should probably make this blank or invalid so folks don't release with these values (inevitably someone will)

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 11, 2016

Contributor

Chatting with @parkr offline, he suggested we remove asset support for a 0.1, since layouts, includes, and sass will all gain core support once #4610 lands, and assets would require custom logic. The intent is follow up with a 0.2 that includes support for assets, but we wanted to get the basic functionality in core.

Once #4610 lands, we'll just have to add something like:

site.includes_dir << theme.includes_path
site.layouts_dir  << theme.layouts_path
site.sass.source  << theme.sass_path

And we should be .

Contributor

benbalter commented Mar 11, 2016

Chatting with @parkr offline, he suggested we remove asset support for a 0.1, since layouts, includes, and sass will all gain core support once #4610 lands, and assets would require custom logic. The intent is follow up with a 0.2 that includes support for assets, but we wanted to get the basic functionality in core.

Once #4610 lands, we'll just have to add something like:

site.includes_dir << theme.includes_path
site.layouts_dir  << theme.layouts_path
site.sass.source  << theme.sass_path

And we should be .

parkr added some commits Mar 25, 2016

Merge remote-tracking branch 'origin/master' into themes
* origin/master: (65 commits)
  Update history to reflect merge of #4703 [ci skip]
  Update history to reflect merge of #4712 [ci skip]
  Highlight the test code
  Update history to reflect merge of #4640 [ci skip]
  readded "env=prod"-condition
  Update history to reflect merge of #3849 [ci skip]
  Update history to reflect merge of #4624 [ci skip]
  Update history to reflect merge of #4704 [ci skip]
  Update history to reflect merge of #4706 [ci skip]
  Checks for link file extension in tests
  Updating assets documentation
  Fix test teardown for cleaner.
  Update history to reflect merge of #4542 [ci skip]
  Add explanation of site variables in the example _config.yml
  Use double quotes in the gemfile
  Add test for creation of Gemfile by 'jekyll new'
  Add comment about github-pages
  Update history to reflect merge of #4533 [ci skip]
  Ensure Rouge closes its div/figure properly after highlighting ends.
  Add Site#config= which can be used to set the config
  ...
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 26, 2016

Member

@benbalter I added all the necessary code –– if you could go ahead and add some tests for the theme stuff, that'd be swell. ❤️

Member

parkr commented Mar 26, 2016

@benbalter I added all the necessary code –– if you could go ahead and add some tests for the theme stuff, that'd be swell. ❤️

@lafraia lafraia referenced this pull request in tomjoht/documentation-theme-jekyll Apr 11, 2016

Closed

Adding support for company_url and company_logo parameters. #19

@benbalter benbalter removed their assignment Apr 11, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Apr 11, 2016

Contributor

@parkr Looking at my week, I'm not going to have the chance to right tests for this in the foreseeable future (but still very much would like to see this 🚢). Unassigning myself in case you or anyone else would like to jump in to unblock 3.2.

Contributor

benbalter commented Apr 11, 2016

@parkr Looking at my week, I'm not going to have the chance to right tests for this in the foreseeable future (but still very much would like to see this 🚢). Unassigning myself in case you or anyone else would like to jump in to unblock 3.2.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 11, 2016

Contributor

I don't see why this should block 3.2? A feature should never block. only bugs.

Contributor

envygeeks commented Apr 11, 2016

I don't see why this should block 3.2? A feature should never block. only bugs.

@mmistakes mmistakes referenced this pull request in mmistakes/minimal-mistakes Apr 18, 2016

Closed

Docs should explain upgrade path #275

parkr added some commits Apr 21, 2016

Merge branch 'master' into themes
* master: (58 commits)
  Update history to reflect merge of #4792 [ci skip]
  Update history to reflect merge of #4793 [ci skip]
  Update history to reflect merge of #4804 [ci skip]
  Update history to reflect merge of #4754 [ci skip]
  Update history to reflect merge of #4813 [ci skip]
  Added missing single quote on rsync client side command
  Add v3.0.4 and v3.1.3 to the history.
  Fixed typo
  Add jekyll-autoprefixer plugin
  Explicitly require Filters rather than implicitly.
  Update history to reflect merge of #4786 [ci skip]
  Update history to reflect merge of #4789 [ci skip]
  updates example domain in config template
  Globalize Jekyll's Filters.
  Update JRuby to 9.0.5.0; Drop the double digit test.
  Update Rack-Jekyll Heroku deployment blog post url
  convertible: use Document::YAML_FRONT_MATTER_REGEXP to parse transformable files
  Update history to reflect merge of #4734 [ci skip]
  Update history to reflect merge of #4478 [ci skip]
  Fix rubocop warning.
  ...
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 22, 2016

Member

@benbalter It's working! 🎉

Member

parkr commented Apr 22, 2016

@benbalter It's working! 🎉

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 22, 2016

Member

@benbalter I updated it to resolve the realpath of the folder so people couldn't get around our symlink checks by shipping a theme with symlinks in them.

Well done, all!

@jekyllbot: merge +minor

Member

parkr commented Apr 22, 2016

@benbalter I updated it to resolve the realpath of the folder so people couldn't get around our symlink checks by shipping a theme with symlinks in them.

Well done, all!

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 30f2bdf into master Apr 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the themes branch Apr 22, 2016

jekyllbot added a commit that referenced this pull request Apr 22, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 22, 2016

@parkr it's a minor enhancement? :/

ghost commented Apr 22, 2016

@parkr it's a minor enhancement? :/

@AndorChen

This comment has been minimized.

Show comment
Hide comment
@AndorChen

AndorChen Apr 22, 2016

But what about plugins? After a quick test, I found Jekyll cannot load plugins in _plugins folder from a theme.

But what about plugins? After a quick test, I found Jekyll cannot load plugins in _plugins folder from a theme.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 22, 2016

Contributor

But what about plugins? After a quick test, I found Jekyll cannot load plugins in _plugins folder from a theme.

@AndorChen: And it should remain so... for now. If you wish to have themes quickly available on Github at your will, and whitelisted, your best option at this point is to disallow plugins from themes, as they pose a risk to Github, and therefore require a more careful eye, which impedes the goal of this pull request. I'm sure in the future your request can be considered (and perhaps you could file a ticket to spark a discussion on it) but for now, the goal here is to give users a large array of themes they can quickly and readily use.

Contributor

envygeeks commented Apr 22, 2016

But what about plugins? After a quick test, I found Jekyll cannot load plugins in _plugins folder from a theme.

@AndorChen: And it should remain so... for now. If you wish to have themes quickly available on Github at your will, and whitelisted, your best option at this point is to disallow plugins from themes, as they pose a risk to Github, and therefore require a more careful eye, which impedes the goal of this pull request. I'm sure in the future your request can be considered (and perhaps you could file a ticket to spark a discussion on it) but for now, the goal here is to give users a large array of themes they can quickly and readily use.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 27, 2016

Member

@parkr it's a minor enhancement? :/

@spudowiar Yep! Minor enhancements indicate a minor bump, major enhancements indicate a major bump. If it were labeled a major enhancement, we'd be on the road to Jekyll 4!

Member

parkr commented Apr 27, 2016

@parkr it's a minor enhancement? :/

@spudowiar Yep! Minor enhancements indicate a minor bump, major enhancements indicate a major bump. If it were labeled a major enhancement, we'd be on the road to Jekyll 4!

@benbalter benbalter referenced this pull request May 13, 2016

Closed

Release Jekyll v3.2 #4891

12 of 12 tasks complete

@haslinger haslinger referenced this pull request May 15, 2016

Closed

Gem-based themes #4510

@SirRawlins

This comment has been minimized.

Show comment
Hide comment
@SirRawlins

SirRawlins Jul 21, 2016

Hey folks, this is a great enhancement.

What's the state of play with _assets in a theme, are they supported yet? are they on a roadmap?

@envygeeks you mention you are running a workaround for this, may I ask about your setup? Happy to chat offline.

Hey folks, this is a great enhancement.

What's the state of play with _assets in a theme, are they supported yet? are they on a roadmap?

@envygeeks you mention you are running a workaround for this, may I ask about your setup? Happy to chat offline.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 21, 2016

I'm currently just calling Jekyll Assets on the folder from a plugin (on the Gem's _assets folder)

ghost commented Jul 21, 2016

I'm currently just calling Jekyll Assets on the folder from a plugin (on the Gem's _assets folder)

@SirRawlins

This comment has been minimized.

Show comment
Hide comment
@SirRawlins

SirRawlins Jul 21, 2016

@spudowiar thanks for the reply. So your theme is utilizing the Jekyll-Assets gem? and that works out the box? You install the theme and the _assets folder becomes available to you?

@spudowiar thanks for the reply. So your theme is utilizing the Jekyll-Assets gem? and that works out the box? You install the theme and the _assets folder becomes available to you?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 21, 2016

@SirRawlins it requires a file to be dropped into _plugins and the jekyll-assets gem

ghost commented Jul 21, 2016

@SirRawlins it requires a file to be dropped into _plugins and the jekyll-assets gem

@SirRawlins

This comment has been minimized.

Show comment
Hide comment
@SirRawlins

SirRawlins Jul 21, 2016

@sparanoid Sorry, wrong person.

@spudowiar Thanks. any chance of a copy of that plugin? Or a gist of what it's doing? Playing around with ideas myself here, I assume it uses a hook to determine the path of the theme and then passes that to the jekyll-assets config?

SirRawlins commented Jul 21, 2016

@sparanoid Sorry, wrong person.

@spudowiar Thanks. any chance of a copy of that plugin? Or a gist of what it's doing? Playing around with ideas myself here, I assume it uses a hook to determine the path of the theme and then passes that to the jekyll-assets config?

@SirRawlins

This comment has been minimized.

Show comment
Hide comment
@SirRawlins

SirRawlins Jul 21, 2016

@spudowiar @envygeeks moved this into Jekyll talk to save polluting the PR. https://talk.jekyllrb.com/t/jekyll-assets-gem-based-theme/2667

@spudowiar @envygeeks moved this into Jekyll talk to save polluting the PR. https://talk.jekyllrb.com/t/jekyll-assets-gem-based-theme/2667

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