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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose site source #107

Merged
merged 2 commits into from
Aug 15, 2017
Merged

Expose site source #107

merged 2 commits into from
Aug 15, 2017

Conversation

benbalter
Copy link
Contributor

...and one more PR to follow up on expose repo license and expose repo visibility.

The idea is still to enable a template as follows:

      {% if site.github.private != true and site.github.license %}
      <div class="footer border-top border-gray-light mt-5 text-right">
        This site is open source. <a href="{{ site.github.repository_url }}/edit/{{ site.github.source.branch }}{{ site.github.source.path }}{{ page.path }}">Help improve this page</a>
      </div>
      {% endif %}

@benbalter benbalter self-assigned this Aug 15, 2017
@benbalter benbalter requested a review from parkr August 15, 2017 18:13
{"message":"Not Found","documentation_url":"https://developer.github.com/v3"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm this should have valid data :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't modify this. Restored the whitespace via 43e425b to clean up the diff.

Based on trying to implement things differently the first time, I believe this is here to ensure that repo compat is exercised for the integration test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Ok cool, looking through the history, it looks like it's trying to fetch data from this repo, which doesn't have Pages enabled so that'd explain it. Womp. Ideally we'd exercise both repo compat and the repo API stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this at first glance. Added unit tests via 7abdfe2.

@parkr
Copy link
Member

parkr commented Aug 15, 2017

I love this!!

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am suuuuuper stoked for this!

One enhancement we might consider is adding a Liquid tag like {%- github_edit_link page -%} which would output an HTML <a> link all built for you. The big reason not to do that is people will want lots of options like setting CSS tags and so forth. Maybe {%- github_edit_url page -%}? 😄 Something to think about.

Thank you!!

@benbalter
Copy link
Contributor Author

@jekyllbot: merge +minor.

@benbalter benbalter merged commit c252a75 into master Aug 15, 2017
@benbalter benbalter deleted the expose-source branch August 15, 2017 19:37
benbalter added a commit that referenced this pull request Aug 15, 2017
@jekyll jekyll locked and limited conversation to collaborators Apr 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants