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

Add SEO data #12

Merged
merged 29 commits into from
Sep 22, 2016
Merged

Add SEO data #12

merged 29 commits into from
Sep 22, 2016

Conversation

Cantido
Copy link
Contributor

@Cantido Cantido commented Sep 5, 2016

Adds Twitter, Opengraph, JSON-LD, and whatever other data we need in order to be relevant on the internet. This will fix #6.

Planned:

  • Description meta tag on main page
  • Keywords meta tag on main page
  • Opengraph meta tags on main page
  • Twitter meta tags on main page
  • Validate robots.txt
  • Create a sitemap.xml Postponed to Generate a Sitemap #39

@Cantido Cantido self-assigned this Sep 5, 2016
@@ -0,0 +1,50 @@
<%# This belongs inside the <head> tag %>

<link rel="publisher" href="https://plus.google.com/u/0/+IndentLabs" />
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to drop the /u/0 here -- it's an identifier for which Google account you are logged into, rather than actually part of the URL.

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 thought so, thanks for clearing that up. I'll use the link that you put on the main page.

@drusepth
Copy link
Member

drusepth commented Sep 5, 2016

@Cantido A couple quick comments, but otherwise this looks rockin'. Thanks!

@drusepth
Copy link
Member

drusepth commented Sep 5, 2016

Looks great. @Cantido Merge at will!

@Cantido
Copy link
Contributor Author

Cantido commented Sep 5, 2016

I'm going to refine this a little bit so that the content pages have metadata as well, so hold off on merging.

Also I don't seem to have authorization to merge this pull request. Seems to be a result of the protected master branch. I'm totally fine with only you having merge access, it's up to you. Just wait for me to say I'm all done lol.

@Cantido
Copy link
Contributor Author

Cantido commented Sep 5, 2016

There's no sitemap for the site either, is there? If not, I'll add this gem as well:

https://github.com/kjvarga/sitemap_generator

@drusepth
Copy link
Member

drusepth commented Sep 5, 2016

Works for me! I don't believe there's a sitemap, I wasn't aware those were
actually still used. If they are though, go for it!

I'll merge whenever you're ready then :)

On Sep 5, 2016 11:13 AM, "Robert Richter" notifications@github.com wrote:

There's no sitemap for the site either, is there? If not, I'll add this gem
as well:

https://github.com/kjvarga/sitemap_generator


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#12 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAg2e5IEcwFyzJFwNw1Zkf0Icm6v_3VZks5qnD-3gaJpZM4J0vgD
.

@Cantido Cantido mentioned this pull request Sep 5, 2016
Since the SEO partial will be in every page, we must keep either data
that is the same for all pages, or pointers to the data that will
be set dynamically in other pages.
@Cantido Cantido added this to the NaNoWriMo 2016 milestone Sep 5, 2016
@@ -1,3 +1,6 @@
<%# Recommended keywords tag length: up to 255 characters, 20 words. %>
<% set_meta_tags keywords: %w[writing author nanowrimo novel character fiction fantasy universe creative dnd roleplay larp game design] %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drusepth These are the keywords I came up with off the top of my head, but I'm sure there are better ones. Feel free to modify this as you wish.

Copy link
Member

Choose a reason for hiding this comment

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

These are good for now. We can always add to them later. 👍

@Cantido
Copy link
Contributor Author

Cantido commented Sep 5, 2016

Heroku does not allow us to modify the app's filesystem after deployment, so we can't generate a sitemap in the same directory. What people usually do is upload their sitemap to an S3 bucket instead. Here's the exact process we would have to follow:

http://jademind.com/blog/posts/updating-sitemap-file-on-heroku/

@Cantido Cantido mentioned this pull request Sep 6, 2016
@drusepth drusepth removed this from the NaNoWriMo 2016 milestone Sep 15, 2016
@Cantido
Copy link
Contributor Author

Cantido commented Sep 20, 2016

We're at a decision point. I've done the following so far:

  • Description meta tag on main page
  • Keywords meta tag on main page
  • Opengraph meta tags on main page
  • Twitter meta tags on main page
  • Validate robots.txt

We have only the "Create a sitemap.xml" task remaining. Since it seems like that would require getting S3 set up, we can postpone that task so I can get to working on something else. @drusepth, what do you say?

@drusepth
Copy link
Member

@Cantido I say lets leave it for later and move forward. There's lots more fun stuff to do than set up S3. 👍

Copy link
Contributor Author

@Cantido Cantido left a comment

Choose a reason for hiding this comment

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

All set to merge. I closed all my script tags!

@Cantido Cantido mentioned this pull request Sep 20, 2016
@Cantido
Copy link
Contributor Author

Cantido commented Sep 20, 2016

That should get Twitter cards properly displayed for all content. I realized that using the set_meta_tags method after you've called display_meta_tags does absolutely nothing. I thought the entire meta tag data structure would be assembled first, before the actual HTML was written.

@@ -16,6 +16,26 @@ class User < ActiveRecord::Base
has_many :magics
has_many :universes

# as_json would try to print the password digest, which requires authentication
def as_json(options={})
excludes = [:password_digest, :old_password]
Copy link
Member

@drusepth drusepth Sep 21, 2016

Choose a reason for hiding this comment

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

Would we also want to exclude encrypted_password and reset_password_token? And maybe email, so we're not exposing user emails to anyone? What do you think?

super(options)
end

def to_json(options={})
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between to_json and as_json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as_json creates a hash structure, which you then pass to ActiveSupport::json.encode to actually encode the object as a JSON string. to_json converts it straight to an escaped JSON string.

end

def to_json(options={})
options[:except] ||= [:password_digest, :old_password]
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to define this list on the model somewhere so we can reuse it and modify it in a single place, instead of hard-coding it in these handful of places.

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 agree, and I'll look for a way to whitelist what we want, instead of blacklisting like this.

<span class="card-title">Your digital notebook is here.</span>
</div>
<div class="card-content">
<h4>
Notebook is a set of tools for writers, game designers, and roleplayers to create magnificent universes &ndash; and everything within them.
<%= description 'Notebook is a set of tools for writers, game designers, and roleplayers to create magnificent universes &ndash; and everything within them.' %>
Copy link
Member

Choose a reason for hiding this comment

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

Where's the description method defined? I've never seen this before!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's provided by the meta-tags gem. It sets the page's description, and then lets the string pass through.

Copy link
Contributor Author

@Cantido Cantido left a comment

Choose a reason for hiding this comment

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

Okay I am much more satisfied with that set of changes. It looks ready to merge.

Copy link
Member

@drusepth drusepth left a comment

Choose a reason for hiding this comment

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

Looks good to merge to me. Great work! 👍


# Attributes that are non-public, and should be blacklisted from any public
# export (ex. in the JSON api, or SEO meta info about the user)
def blacklisted_attributes
Copy link
Member

Choose a reason for hiding this comment

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

We should probably revisit whitelisting these (or at least verify it's not exposing anything it shouldn't), but this is fine for now. 👍

<% title @content.name %>
<% if @content.present? && @content.respond_to?(:as_jsonld) %>
<script type="application/ld+json">
<%= @content.as_jsonld.to_json.html_safe %>
Copy link
Member

Choose a reason for hiding this comment

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

TOO MUCH WHITESPACE HERE LITERALLY UNMERGEABLE

@drusepth drusepth merged commit 3b4208f into master Sep 22, 2016
@drusepth drusepth deleted the issue-6-seo branch September 22, 2016 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

No SEO
2 participants