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

Adding OG and other meta tags to head #2033

Merged
merged 17 commits into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@tbaxter-18f
Member

tbaxter-18f commented Jul 2, 2018

I pondered different approaches to this for quite a while. If anyone has concerns about this approach, holler.

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jul 2, 2018

Currently blocked on merging in #2029

tbaxter-18f and others added some commits Jul 2, 2018

@hbillings

This approach makes sense to me! I really like the ability to override the values if necessary, without exposing them unless the user is actually looking to change them. The sensible defaults are 👌 too.

from .sample_users import SAMPLE_USERS
def canonical_url(request):
'''Include the request's canonical URL in all request contexts'''
return {'canonical_url': get_canonical_url(request)}

This comment has been minimized.

@hbillings

hbillings Jul 5, 2018

Member

I thought this was necessary for creating the right URLs in automatically generated emails and slack posts, but I can't trace it back to those things now, so 👍

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jul 5, 2018

Member

canonical URLs are pretty good to have, especially when your system can do a lot of redirects or have a lot of routes resolving to the same content (both of which Django will happily let you do). I only removed it here because I rolled the functionality into the template tag rather than as a separate context processor. My thinking is that it made sense to centralize any code inserting into head

Also worth pointing out: The get_canonical_url method is still where it was in utils (for use by the emails). This just removes the context processor since we're not relying on it in base.html any more.

You can also use named arguments:
{% head_meta title=title %} or
{% head_meta title="My most excellent page" %}

This comment has been minimized.

@hbillings

hbillings Jul 5, 2018

Member

Copacetic, dude ☮️

This comment has been minimized.

@tbaxter-18f
@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jul 6, 2018

I should mention that my biggest concern with this is that it encourages you to do the wrong thing and fall back to the default title and description on all pages. Each public-facing page should have a unique title and description, but this would require you to remember to set that in a not-particularly-clear way.

@tbaxter-18f tbaxter-18f changed the title from WIP: Adding OG and other meta tags to head to Adding OG and other meta tags to head Jul 10, 2018

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jul 10, 2018

OK, I think this is ready for f'realsies review now. Tests are passing locally.

@hbillings

This comment has been minimized.

Member

hbillings commented Jul 11, 2018

WHOOOOOOOOOO

@tbaxter-18f tbaxter-18f merged commit d490bec into develop Jul 11, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
security/snyk - package.json (CALC) No manifest changes detected
security/snyk - requirements.txt (CALC) No manifest changes detected

@tbaxter-18f tbaxter-18f deleted the 135-og-data branch Jul 11, 2018

@ericronne ericronne referenced this pull request Jul 11, 2018

Closed

Add OG metadata #135

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment