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

Clean up Open Graph meta tags #298

Merged
merged 8 commits into from Feb 27, 2022
Merged

Clean up Open Graph meta tags #298

merged 8 commits into from Feb 27, 2022

Conversation

joemasilotti
Copy link
Owner

This PR cleans up how/when Open Graph tags are rendered and tweaks the copy on a few key pages.

Current failing tests are because I can't figure out how to test content_for in a View Component.

Pull request checklist

  • Your code contains tests relevant for code you modified
  • You have linted and tested the project with bin/check

@@ -1,5 +1,6 @@
module MetaHelper
def open_graph_tags
content_for :open_graph_tags
render(OpenGraphTagsComponent.new) unless content_for?(:open_graph_tags)
content_for(:open_graph_tags)
Copy link

@radar radar Feb 25, 2022

Choose a reason for hiding this comment

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

Following from your Twitter thread here: https://twitter.com/joemasilotti/status/1497341331315650560?s=20&t=nc7y6ITf4HnEDPGLhai9LA.

How about:

render(OpenGraphTagsComponent.new, tags: content_for(:open_graph_tags))

Then in the component itself, return the tags if they're present, otherwise render the component's view. This would remove the need for content_for use in the component's view, and the code here then is just that single render line.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe I'm missing something, but doesn't this mean I have to add back in the following block every time I render the component?

<% content_for(:open_graph_tags) do %>
  <%= render OpenGraphComponent.new(...) %>
<% end %>

Choose a reason for hiding this comment

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

I think I prefer the content_for block in every view anyways. It keeps the component simple and more clear in the view what is happening. Why are you trying to remove the content_for block from all the views?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because it feels like a responsibility every single view shouldn't have. The rendering helper and the component already know about the magic symbol, I'd rather not spread it across the rest of the codebase too.

There's probably a different abstraction I'm missing here, to be honest.

@joemasilotti
Copy link
Owner Author

OK I think I have something working locally! I'll commit later today but the gist is to render the content_for in a helper, not the component.

@joemasilotti joemasilotti changed the title Clean up og meta tags Clean up Open Graph meta tags Feb 27, 2022
@joemasilotti
Copy link
Owner Author

For those following along, I went with something a bit different. Instead of rendering the content_for in the component I've moved that logic to a helper that then renders the component. The helper passes all the arguments down to the component and has an option to render a more specific Open Graph component.

def open_graph_tags(options = {})
  content_for(:open_graph_tags) do
    component = options.delete(:component) || OpenGraphTagsComponent
    render component.new(**options)
  end
end

@joemasilotti joemasilotti merged commit f1e6524 into main Feb 27, 2022
5 checks passed
@joemasilotti joemasilotti deleted the clean-up-og-meta-tags branch February 27, 2022 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants