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

Added new includes.md topic to docs #5696

Merged
merged 11 commits into from Jan 8, 2017

Conversation

Projects
None yet
6 participants
@tomjoht
Contributor

tomjoht commented Dec 26, 2016

Added new includes.md topic.

See #5630 for more details on the update.

@jekyll/documentation

tomjoht added some commits Dec 26, 2016

Add includes link to doc nav
I created more advanced details about includes and created a new page for it instead of putting all the info on the templates page.

See #5630 for more details on the update. 

@jekyll/documentation
@DirtyF
Added new includes.md topic to docs
Added new includes.md topic.

See #5630 for more details on the update. 

@jekyll/documentation
@DirtyF
@DirtyF

DirtyF requested changes Dec 26, 2016 edited

Could you add spaces for variables, e.g {{ page.my_variable }} instead of {{page.my_variable}}

Show outdated Hide outdated docs/_docs/includes.md
{% raw %}<div markdown="span" class="alert alert-info" role="alert">
<i class="fa fa-info-circle"></i> <b>Note:</b> {{include.content}}
</div>{% endraw %}
```

This comment has been minimized.

@ashmaroli

ashmaroli Dec 26, 2016

Member

the raw tags would read better on GitHub when on dedicated lines:

{% raw %}
  <div markdown="span" class="alert alert-info" role="alert">
  <i class="fa fa-info-circle"></i> <b>Note:</b> {{ include.content }}
  </div>
{% endraw %}
@ashmaroli

ashmaroli Dec 26, 2016

Member

the raw tags would read better on GitHub when on dedicated lines:

{% raw %}
  <div markdown="span" class="alert alert-info" role="alert">
  <i class="fa fa-info-circle"></i> <b>Note:</b> {{ include.content }}
  </div>
{% endraw %}

This comment has been minimized.

@ashmaroli

ashmaroli Dec 26, 2016

Member

update to the comment above:
"HTML can split to a newline for better readability:"

{% raw %}
  <div markdown="span" class="alert alert-info" role="alert">
    <i class="fa fa-info-circle"></i> 
    <b>Note:</b> 
    {{ include.content }}
  </div>
{% endraw %}

once Jekyll migrates to Liquid v4, we'll use its new whitespace-control syntax to remove the resulting clear lines from tags.

@ashmaroli

ashmaroli Dec 26, 2016

Member

update to the comment above:
"HTML can split to a newline for better readability:"

{% raw %}
  <div markdown="span" class="alert alert-info" role="alert">
    <i class="fa fa-info-circle"></i> 
    <b>Note:</b> 
    {{ include.content }}
  </div>
{% endraw %}

once Jekyll migrates to Liquid v4, we'll use its new whitespace-control syntax to remove the resulting clear lines from tags.

This comment has been minimized.

@tomjoht

tomjoht Dec 27, 2016

Contributor

I can split up the HTML onto separate lines, but if you put the raw tags on their own lines, they create a blank space there, which makes the pre formatting look like it has a bunch of empty space at the top and bottom.

@tomjoht

tomjoht Dec 27, 2016

Contributor

I can split up the HTML onto separate lines, but if you put the raw tags on their own lines, they create a blank space there, which makes the pre formatting look like it has a bunch of empty space at the top and bottom.

This comment has been minimized.

@tomjoht

tomjoht Dec 27, 2016

Contributor

re the space around tags such as {{ include.content }}, I made this update. At some point, i'd like to create a style guide for the site that defines conventions like these.

@tomjoht

tomjoht Dec 27, 2016

Contributor

re the space around tags such as {{ include.content }}, I made this update. At some point, i'd like to create a style guide for the site that defines conventions like these.

This comment has been minimized.

@ashmaroli

ashmaroli Dec 27, 2016

Member

but if you put the raw tags on their own lines, they create a blank space there, which makes the pre formatting look like it has a bunch of empty space at the top and bottom.

Hence the reason why i mentioned about Liquid 4's ability to remove that whitespace..

@ashmaroli

ashmaroli Dec 27, 2016

Member

but if you put the raw tags on their own lines, they create a blank space there, which makes the pre formatting look like it has a bunch of empty space at the top and bottom.

Hence the reason why i mentioned about Liquid 4's ability to remove that whitespace..

This comment has been minimized.

@ashmaroli

ashmaroli Dec 27, 2016

Member

At some point, i'd like to create a style guide for the site that defines conventions like these.

Ben has already set of guidelines defined at https://github.com/benbalter/jekyll-style-guide. Add to it if not already defined..

@ashmaroli

ashmaroli Dec 27, 2016

Member

At some point, i'd like to create a style guide for the site that defines conventions like these.

Ben has already set of guidelines defined at https://github.com/benbalter/jekyll-style-guide. Add to it if not already defined..

Show outdated Hide outdated docs/_docs/includes.md
</div>{% endraw %}
```
The {% raw %}`{{include.content}}`{% endraw %} is a parameter gets populated when you call the include and specify a value for that parameter, like this:

This comment has been minimized.

@ashmaroli

ashmaroli Dec 26, 2016

Member
  • raw tags not required for inline-codes.. {{include.content}} is sufficient
    Update: Apparently, raw tags are required...
  • whitespace around parameter.. {{ include.content }} is the recommended syntax
@ashmaroli

ashmaroli Dec 26, 2016

Member
  • raw tags not required for inline-codes.. {{include.content}} is sufficient
    Update: Apparently, raw tags are required...
  • whitespace around parameter.. {{ include.content }} is the recommended syntax

This comment has been minimized.

@tomjoht

tomjoht Dec 27, 2016

Contributor

updated

@tomjoht

tomjoht Dec 27, 2016

Contributor

updated

made requested updates
- made `{{ }}` formatting more readable by adding spacing. 
- added formatting to code samples to properly reflect line breaks for readability
@tomjoht

This comment has been minimized.

Show comment
Hide comment
@tomjoht

tomjoht Dec 27, 2016

Contributor

I made all the updates requested on this topic, so you can re-review and potentially merge the patch now.

Contributor

tomjoht commented Dec 27, 2016

I made all the updates requested on this topic, so you can re-review and potentially merge the patch now.

moved includes to appear after templates
made update based on review. moved includes topic after templates in nav list.
Show outdated Hide outdated docs/_docs/includes.md
{% if {{ include.caption }} %}
<figcaption>{{ include.caption }}</figcaption>
{% endif %}
</figure>{% endraw %}

This comment has been minimized.

@ashmaroli

ashmaroli Dec 27, 2016

Member

I'm having a hard-time reading the code here (slight exaggeration).. consider indenting lines as required..

you cant have {{ }} inside {% %} have you tested it locally?

@ashmaroli

ashmaroli Dec 27, 2016

Member

I'm having a hard-time reading the code here (slight exaggeration).. consider indenting lines as required..

you cant have {{ }} inside {% %} have you tested it locally?

This comment has been minimized.

@tomjoht

tomjoht Dec 27, 2016

Contributor

ahh, now we're coming into something interesting territory. I tested the code locally and have been using includes like this for months in my doc projects. But I didn't have spaces around the variables. It turns out this spacing matters when the variable appears inside an if statement.

This works:

<figure>
   {% if {{include.url}} %}<a href="{{include.url}}">{% endif %}
   <img src="{{include.file}}" {% if {{include.max-width}} %}
   style="max-width: {{include.max-width }}" {% endif %}
   alt="{{include.alt}}" />{% if {{include.url}} %}</a>{% endif %}
   {% if {{include.caption}} %}
   <figcaption>{{include.caption}}</figcaption>
   {% endif %}
</figure>

This doesn't:

<figure>
   {% if {{ include.url }} %}<a href="{{ include.url }}">{% endif %}
   <img src="{{ include.file }}" {% if {{ include.max-width }} %}
   style="max-width: {{ include.max-width }}" {% endif %}
   alt="{{ include.alt }}" />{% if {{ include.url }} %}</a>{% endif %}
   {% if {{ include.caption }} %}
   <figcaption>{{ include.caption }}</figcaption>
   {% endif %}
</figure>

The difference is with the spacing in the page variables.

I was under the impression that {% if {{include.url}} %} and {% if {{ include.url }} %} would be rendered the same, since having the include outside of an {% if ... %} doesn't impose the same requirements about spacing. Outside of if statements, {{ include.file }} and {{include.file}} work equally well.

I can remove the spacing for variables inside of if statements in the code and add a little explanatory note. Is this a bug in Jekyll, or a feature?

@tomjoht

tomjoht Dec 27, 2016

Contributor

ahh, now we're coming into something interesting territory. I tested the code locally and have been using includes like this for months in my doc projects. But I didn't have spaces around the variables. It turns out this spacing matters when the variable appears inside an if statement.

This works:

<figure>
   {% if {{include.url}} %}<a href="{{include.url}}">{% endif %}
   <img src="{{include.file}}" {% if {{include.max-width}} %}
   style="max-width: {{include.max-width }}" {% endif %}
   alt="{{include.alt}}" />{% if {{include.url}} %}</a>{% endif %}
   {% if {{include.caption}} %}
   <figcaption>{{include.caption}}</figcaption>
   {% endif %}
</figure>

This doesn't:

<figure>
   {% if {{ include.url }} %}<a href="{{ include.url }}">{% endif %}
   <img src="{{ include.file }}" {% if {{ include.max-width }} %}
   style="max-width: {{ include.max-width }}" {% endif %}
   alt="{{ include.alt }}" />{% if {{ include.url }} %}</a>{% endif %}
   {% if {{ include.caption }} %}
   <figcaption>{{ include.caption }}</figcaption>
   {% endif %}
</figure>

The difference is with the spacing in the page variables.

I was under the impression that {% if {{include.url}} %} and {% if {{ include.url }} %} would be rendered the same, since having the include outside of an {% if ... %} doesn't impose the same requirements about spacing. Outside of if statements, {{ include.file }} and {{include.file}} work equally well.

I can remove the spacing for variables inside of if statements in the code and add a little explanatory note. Is this a bug in Jekyll, or a feature?

This comment has been minimized.

@ashmaroli

ashmaroli Dec 28, 2016

Member

removing the space around variables may "work" within {% if } blocks but Liquid should be issuing a warning it's wrong nonetheless..
I'm not in favor of Jekyll's official documentation stating this.

/cc @pathawks your opinion?

@ashmaroli

ashmaroli Dec 28, 2016

Member

removing the space around variables may "work" within {% if } blocks but Liquid should be issuing a warning it's wrong nonetheless..
I'm not in favor of Jekyll's official documentation stating this.

/cc @pathawks your opinion?

This comment has been minimized.

@tomjoht

tomjoht Dec 28, 2016

Contributor

I think it would be best if I just removed the section about conditionalizing the includes. I can take out this part: "You could also use if tags to safeguard scenarios where either the parameter is optional or where the author doesn't include the parameter:....."

I'm guessing the correct approach is to use the default filter, right?

If this really should be an error from Liquid, then yeah, the jekyll docs shouldn't promote bad code.

I'll also omit the paragraph I added about spacing versus non-spacing and errors.

@tomjoht

tomjoht Dec 28, 2016

Contributor

I think it would be best if I just removed the section about conditionalizing the includes. I can take out this part: "You could also use if tags to safeguard scenarios where either the parameter is optional or where the author doesn't include the parameter:....."

I'm guessing the correct approach is to use the default filter, right?

If this really should be an error from Liquid, then yeah, the jekyll docs shouldn't promote bad code.

I'll also omit the paragraph I added about spacing versus non-spacing and errors.

Show outdated Hide outdated docs/_docs/includes.md
</div>{% endraw %}
```
The {% raw %}`{{include.content}}`{% endraw %} is a parameter gets populated when you call the include and specify a value for that parameter, like this:
The `{% raw %}{{ include.content }}{% endraw %}` is a parameter gets populated when you call the include and specify a value for that parameter, like this:

This comment has been minimized.

@ashmaroli

ashmaroli Dec 27, 2016

Member

I made a mis-judgement regarding this the last time.. please revert it..
{% raw %}{{ include.content }}{% endraw %} and adjust all other instances for consistency, (if any)

@ashmaroli

ashmaroli Dec 27, 2016

Member

I made a mis-judgement regarding this the last time.. please revert it..
{% raw %}{{ include.content }}{% endraw %} and adjust all other instances for consistency, (if any)

This comment has been minimized.

@tomjoht

tomjoht Dec 27, 2016

Contributor

can you remind me what the misjudgement was? I simply forgot to put the raw tags inside the backticks. it renders fine.

@tomjoht

tomjoht Dec 27, 2016

Contributor

can you remind me what the misjudgement was? I simply forgot to put the raw tags inside the backticks. it renders fine.

This comment has been minimized.

@ashmaroli

ashmaroli Dec 28, 2016

Member

never mind..

@ashmaroli

ashmaroli Dec 28, 2016

Member

never mind..

Updated to remove spacing from include variables
It turns out Liquid throws an error when you write `{% if {{ include.url }} %}` instead of `{% if {{include.url}} %}`. I updated the examples here to omit the spacing. To avoid inconsistency, I just omitted the spacing from all curly braces. Also added a note explaining the issue and put the blame on Liquid.
@tomjoht

This comment has been minimized.

Show comment
Hide comment
@tomjoht

tomjoht Dec 27, 2016

Contributor

I updated the doc to remove the problematic spacing. Submitting for re-review.

Contributor

tomjoht commented Dec 27, 2016

I updated the doc to remove the problematic spacing. Submitting for re-review.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 28, 2016

Member

I have a request.

When we are reviewing, can we try really hard to perform one thorough review, and then approve once all suggested changes have been made? I recognize that this is more work for the reviewer, but @tomjohnson1492 has put a lot of work in improving our documentation, and I don't want those changes to be lost to bureaucracy.

Do point out things that must be changed before the PR can be merged, but also recognize that things can be further refined in future pull requests. There are some much needed improvements in these pull requests, and I would like to see them land in our documentation as soon as they can be reviewed.

Member

pathawks commented Dec 28, 2016

I have a request.

When we are reviewing, can we try really hard to perform one thorough review, and then approve once all suggested changes have been made? I recognize that this is more work for the reviewer, but @tomjohnson1492 has put a lot of work in improving our documentation, and I don't want those changes to be lost to bureaucracy.

Do point out things that must be changed before the PR can be merged, but also recognize that things can be further refined in future pull requests. There are some much needed improvements in these pull requests, and I would like to see them land in our documentation as soon as they can be reviewed.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 28, 2016

Member

I have a request.

okay @pathawks I'll make sure I'll do a thorough scrutiny before submitting the review

Member

ashmaroli commented Dec 28, 2016

I have a request.

okay @pathawks I'll make sure I'll do a thorough scrutiny before submitting the review

Removed erroneous liquid code, added back spacing
- removed erroneous liquid code with conditional include parameters
- added back spacing in {{ }} tags
@tomjoht

This comment has been minimized.

Show comment
Hide comment
@tomjoht

tomjoht Dec 28, 2016

Contributor

I made 2 fixes here:

  • removed the erroneous liquid code and hack note
  • added back the spacing inside curly braces

Submitting for review.

Contributor

tomjoht commented Dec 28, 2016

I made 2 fixes here:

  • removed the erroneous liquid code and hack note
  • added back the spacing inside curly braces

Submitting for review.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 28, 2016

Member

Since I do not have write-access to the repo, my reviews should not block the merge 😇

Member

ashmaroli commented Dec 28, 2016

Since I do not have write-access to the repo, my reviews should not block the merge 😇

@ashmaroli

LGTM! ❤️ 😃

tomjoht and others added some commits Dec 26, 2016

Added new includes.md topic to docs
Added new includes.md topic.

See #5630 for more details on the update. 

@jekyll/documentation
@DirtyF
made requested updates
- made `{{ }}` formatting more readable by adding spacing. 
- added formatting to code samples to properly reflect line breaks for readability
Updated to remove spacing from include variables
It turns out Liquid throws an error when you write `{% if {{ include.url }} %}` instead of `{% if {{include.url}} %}`. I updated the examples here to omit the spacing. To avoid inconsistency, I just omitted the spacing from all curly braces. Also added a note explaining the issue and put the blame on Liquid.
Removed erroneous liquid code, added back spacing
- removed erroneous liquid code with conditional include parameters
- added back spacing in {{ }} tags
@DirtyF

DirtyF approved these changes Dec 29, 2016

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Jan 8, 2017

Member

@jekyllbot: merge +docs

Member

DirtyF commented Jan 8, 2017

@jekyllbot: merge +docs

@DirtyF DirtyF closed this Jan 8, 2017

@jekyllbot jekyllbot merged commit 0bc4f2a into jekyll:master Jan 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Jan 8, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 8, 2017

Member

🎉

Member

pathawks commented Jan 8, 2017

🎉

@parkr parkr referenced this pull request Jan 9, 2017

Merged

Improve template docs #5694

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