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.

Sign up for GitHub

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

Jump to bottom

Use localhost:4000 as the default pages host in development #50

Merged
merged 5 commits into from
Aug 11, 2016
Merged

Use localhost:4000 as the default pages host in development #50

merged 5 commits into from
Aug 11, 2016

Conversation

benbalter
Copy link
Contributor

Fixes #47.

@benbalter benbalter self-assigned this Mar 30, 2016
@parkr
Copy link
Member

parkr commented Apr 14, 2016

@benbalter I thought PAGES_ENV and JEKYLL_ENV were different things? Why would we conflate them?

@benbalter
Copy link
Contributor Author

I thought PAGES_ENV and JEKYLL_ENV were different things? Why would we conflate them?

Explained my understanding in #49 (comment). Would love to hear your thoughts there, as I suspect the two discussions are really one.

@benbalter benbalter removed their assignment Apr 14, 2016
parkr
parkr reviewed Apr 25, 2016
View reviewed changes
spec/pages_spec.rb
@@ -33,21 +33,38 @@
end
end

it "picks up on JEKYLL_ENV" do
with_env "PAGES_ENV", "" do
with_env("JEKYLL_ENV", "halp") do
Copy link
Member

Choose a reason for hiding this comment

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

Consistent parentheses please 😄

@benbalter
Copy link
Contributor Author

@parkr now that tests pass, how does this look to you?

parkr
parkr reviewed Aug 11, 2016
View reviewed changes
lib/jekyll-github-metadata/pages.rb

def custom_domains_enabled?
dotcom? || test?
end

def env
env_var 'PAGES_ENV'
env_var 'PAGES_ENV', ENV['JEKYLL_ENV']
end
Copy link
Member

Choose a reason for hiding this comment

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

Still feeling dubious about this but 🤷

@parkr
Copy link
Member

parkr commented Aug 11, 2016

LGTM

@benbalter
Copy link
Contributor Author

LGTM.

@benbalter
Copy link
Contributor Author

@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit 49e838c into master Aug 11, 2016
@jekyllbot jekyllbot deleted the development-host branch August 11, 2016 16:21
jekyllbot added a commit that referenced this pull request Aug 11, 2016
@jekyllbot
Update history to reflect merge of #50 [ci skip]
33d5bab
@boturnbow boturnbow mentioned this pull request Aug 30, 2016
Closed
@jekyll jekyll locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
feature frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Properly set site.github.url when in Jekyll.development
3 participants
@benbalter @parkr @jekyllbot