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 a note on `:jekyll_plugins` group in the docs #6488

Merged
merged 4 commits into from Oct 27, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Oct 27, 2017

@jekyllbot jekyllbot requested review from DirtyF, mattr- and parkr Oct 27, 2017

@DirtyF

I can not vouch for all the exposed details here, all I know is that Jekyll do Bundler.require on the :jekyll_plugins group.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Oct 27, 2017

Member

As we require Bundler, should we recommend this as the default ? I mean you just list your gems in :jekyll_plugins and you're done. IIUC only GitHub Pages users have to rely on the plugins key in the _config.ymlbecause the Gemfileis ignored.

Member

DirtyF commented Oct 27, 2017

As we require Bundler, should we recommend this as the default ? I mean you just list your gems in :jekyll_plugins and you're done. IIUC only GitHub Pages users have to rely on the plugins key in the _config.ymlbecause the Gemfileis ignored.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 27, 2017

Member

should we recommend this as the default

Since the documentation already states that these three options are equally potent and intermixable, I don't think one of those options can be a default..

Member

ashmaroli commented Oct 27, 2017

should we recommend this as the default

Since the documentation already states that these three options are equally potent and intermixable, I don't think one of those options can be a default..

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Oct 27, 2017

Member

As we require Bundler, should we recommend this as the default?

We should probably document the order of precedence each one has rather than recommending a default and just leave it at that.

Member

mattr- commented Oct 27, 2017

As we require Bundler, should we recommend this as the default?

We should probably document the order of precedence each one has rather than recommending a default and just leave it at that.

@mattr-

I think this is a good first step towards getting how we handle plugins documented. I have a few things I'd like to see addressed before giving this the green light

Show outdated Hide outdated docs/_docs/plugins.md Outdated
Show outdated Hide outdated docs/_docs/plugins.md Outdated
Show outdated Hide outdated docs/_docs/plugins.md Outdated
Show outdated Hide outdated docs/_docs/plugins.md Outdated
Show outdated Hide outdated docs/_docs/plugins.md Outdated
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 27, 2017

Member

I have a few things I'd like to see addressed

...done.

Member

ashmaroli commented Oct 27, 2017

I have a few things I'd like to see addressed

...done.

@mattr-

🎇 Great work! One small tiny nitpick-y change and this is 👍

Show outdated Hide outdated docs/_docs/plugins.md Outdated
@mattr-

mattr- approved these changes Oct 27, 2017

@ashmaroli This is fantastic. Thanks!

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 27, 2017

Member

Thanks Matt 😃

Member

ashmaroli commented Oct 27, 2017

Thanks Matt 😃

@DirtyF

Thanks @ashmaroli and @matt for making this more clear 💎

Show outdated Hide outdated docs/_docs/plugins.md Outdated
@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Oct 27, 2017

Member

@jekyllbot: merge +docs

Member

DirtyF commented Oct 27, 2017

@jekyllbot: merge +docs

@jekyllbot jekyllbot merged commit a1d45f9 into jekyll:master Oct 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
<p>
Gems included in the <code>:jekyll-plugins</code> group are activated
regardless of the <code>--safe</code> mode setting. Be careful of what
gems are included under this group!

This comment has been minimized.

@parkr

parkr Oct 27, 2017

Member

I'm a little weary of the FUD (fear, uncertainty, and doubt) that "Be careful!" leads to. Could we rephrase this to be less fear-mongery? "Be aware" perhaps?

@parkr

parkr Oct 27, 2017

Member

I'm a little weary of the FUD (fear, uncertainty, and doubt) that "Be careful!" leads to. Could we rephrase this to be less fear-mongery? "Be aware" perhaps?

This comment has been minimized.

@DirtyF

DirtyF Oct 27, 2017

Member

oops didn't see you comment, but totally agree with you. Will fix this right away

@DirtyF

DirtyF Oct 27, 2017

Member

oops didn't see you comment, but totally agree with you. Will fix this right away

This comment has been minimized.

@DirtyF

DirtyF Oct 27, 2017

Member

fixed in 52c3406

@DirtyF

DirtyF Oct 27, 2017

Member

fixed in 52c3406

This comment has been minimized.

@mattr-

mattr- Oct 28, 2017

Member

I didn't even notice this. 🙈 Thanks for pointing it out @parkr.

@mattr-

mattr- Oct 28, 2017

Member

I didn't even notice this. 🙈 Thanks for pointing it out @parkr.

@ashmaroli ashmaroli deleted the ashmaroli:jekyll-plugins-docs branch Oct 27, 2017

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