-
Notifications
You must be signed in to change notification settings - Fork 311
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
[WIP] Federalist prep #1430
[WIP] Federalist prep #1430
Conversation
@@ -2,12 +2,13 @@ module Jekyll | |||
module TeamFilter | |||
def team_photo(name) | |||
person = finder(name) | |||
baseurl = Jekyll.sites[0].config['baseurl'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Aaand Travis failed showing all our internal links as broken. Interesting. Looks like I have a todo list for tomorrow. |
@@ -97,8 +98,9 @@ def lookup(input, args) | |||
def team_link(input) | |||
team = Jekyll.sites[0].collections['team'].docs | |||
index = team.find_index {|x| x.data['name'] == input} | |||
baseurl = Jekyll.sites[0].config['baseurl'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless assignment to variable - baseurl
.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
full_name = team[index].data['full_name'] | ||
string = "<a href=#{url}>#{full_name}</a>" | ||
string = "<a href='#{url}'>#{full_name}</a>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless assignment to variable - string
.
@@ -38,7 +39,7 @@ def list_posts(other_posts) | |||
internal = @params[1] || "li" | |||
if other_posts | |||
related_posts = "<#{external}>" | |||
other_posts.flatten.map { |post| related_posts << "<#{internal}><a href='#{post.url}' class='related_posts'>“#{post.data['title']}”</a></#{internal}>" } | |||
other_posts.flatten.map { |post| related_posts << "<#{internal}><a href='#{@baseurl}#{post.url}' class='related_posts'>“#{post.data['title']}”</a></#{internal}>" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [183/80]
Since the post.url appends a / we don't need to separate it from baseurl with one.
OK. I think this is ready to merge. Before making the full cutover we need to:
|
@sharms is working on helping the dashboard team move to dashboard.18f.gov btw |
Awesome. @sharms look forward to working with you on that :) Work has begun here: 18F/dashboard#339 happy to discuss our hopes and dreams, fears and anxieties, there. |
OK, the trailing slash thing I think is a problem only with the preview URLs. If you go to the S3 bucket directly, the downloading problem doesn't happen. Hat tip to @shawnbot for helping figure this out. If you navigate directly to |
As a note, another thing we should bear in mind about moving to ELBs is we will not be able to do OCSP stapling. The lack of stapling is what causes the unencrypted OCSP request to ocsp.x1-int.letsencrypt.org here: http://www.webpagetest.org/result/160113_2S_1207/1/details/ It's also one of the third party services I originally proposed documenting. Stapling for 18f.gsa.gov only stalled because, at the time we were talking about it, nginx had done a very confusing and incomplete job of implementing it, but that has been addressed in recent versions of nginx and there are clear best practices now. It's up to Amazon to enable OCSP stapling for ELBs, and I haven't heard anything from them about doing this. They do have it enabled for CloudFront. |
We can safely merge this without affecting the existing hosting environment. That will at least give us the advantage of preview branches and potentially the editor while we figure out how to move forward on these remaining issues. |
@@ -10,7 +10,7 @@ | |||
{% for post in page.posts %} | |||
<article class="tag-index"> | |||
<div class="blog-title"> | |||
<h1 itemprop="headline" class="h4"><a class="link-alt" href="{{ post.url }}">{{ post.title }}</a></h1> | |||
<h1 itemprop="headline" class="h4"><a class="link-alt" a href="{{ site.baseurl }}{{ post.url }}">{{ post.title }}</a></h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous a
attribute in the anchor
tag
Nice work @gboone. Looks great to me! 🚢 🚀 |
@gboone to close out the rest of this conversation, the hosting piece, I set up the proxy. It's available here: https://18f-site.18f.gov/ It's running as the app To switch the main domain to this proxy, there are two steps:
Also the proxy repo is now here: https://github.com/18F/cg-s3-proxy and is deployed with the following ENV vars:
|
Also, at that point, you may want to switch your default branch in Federalist from |
This pull request preps 18f.gsa.gov to use the Federalist platform. The short term goal is to use the previewing feature for, among other things, blog post drafting. The long term goal is to fully host the site on the platform. Outstanding issues blocking that include:
{{ site.baseurl }}/dashboard/
or dashboard.18f.govbaseurl
-less URLs, images, links, scripts, etc. kicking around.{{ site.baseurl }}/team
vs{{ site.baseurl }}/team/
)Preview branch: https://federalist.18f.gov/preview/18F/18f.gsa.gov/federalist-prep/