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 page_description helper, refactor internals #47
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
module Flutie | ||
module ViewHelper | ||
def body_class | ||
basic_body_class = "#{controller_name} #{controller_name}-#{action_name}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this generate something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that |
||
|
||
if content_for?(:extra_body_classes) | ||
[basic_body_class, content_for(:extra_body_classes)].join(' ') | ||
else | ||
basic_body_class | ||
end.gsub('/', '-') | ||
end | ||
|
||
def page_title | ||
content_tag :title, translate_by_controller_action(:page_title) | ||
end | ||
|
||
def page_description | ||
tag :meta, | ||
{ | ||
content: translate_by_controller_action(:page_description), | ||
name: 'description' | ||
}, | ||
true | ||
end | ||
|
||
def translate_by_controller_action(tag) | ||
key = [controller_name, action_name, tag].join('.').gsub('/', '-') | ||
I18n.t(key, default: '') | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
require 'flutie/version' | ||
require 'rails/engine' | ||
require 'flutie/engine' | ||
require 'helpers/flutie/view_helper' | ||
|
||
module Flutie | ||
require 'flutie/railtie' if defined?(Rails) | ||
end |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
module Flutie | ||
class Engine < Rails::Engine | ||
end | ||
end |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
require 'rspec/expectations' | ||
require 'i18n' | ||
require 'action_view' | ||
require File.expand_path('../../../app/helpers/flutie/view_helper', __FILE__) | ||
|
||
module Flutie | ||
module Stubs | ||
def stub_controller_action(controller, action) | ||
helper.stub(controller_name: controller, action_name: action) | ||
helper.stub(content_for?: false) | ||
end | ||
|
||
def store_translations(translations) | ||
I18n.backend.store_translations(:en, translations) | ||
end | ||
|
||
def helper | ||
@helper ||= Class.new { include Flutie::ViewHelper }.new | ||
end | ||
end | ||
|
||
module ViewHelper | ||
include ActionView::Helpers::TagHelper | ||
end | ||
end | ||
|
||
RSpec.configure do |config| | ||
config.include Flutie::Stubs | ||
config.include RSpec::Matchers | ||
config.mock_framework = :rspec | ||
end | ||
|
||
describe Flutie::ViewHelper do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be nice to put this in its own file, but maybe that's for another PR? |
||
describe '#body_class' do | ||
it 'handles normal controllers' do | ||
stub_controller_action('widgets', 'show') | ||
|
||
result = helper.body_class | ||
|
||
expect(result).to eq 'widgets widgets-show' | ||
end | ||
|
||
it 'handles nested controllers' do | ||
stub_controller_action('module/widgets', 'show') | ||
|
||
result = helper.body_class | ||
|
||
expect(result).to eq 'module-widgets module-widgets-show' | ||
end | ||
|
||
it 'handles extra body classes set via content_for' do | ||
stub_controller_action(:widgets, :show) | ||
helper.stub(content_for?: true) | ||
helper.stub(content_for: 'extra classes') | ||
|
||
result = helper.body_class | ||
|
||
expect(result).to eq 'widgets widgets-show extra classes' | ||
end | ||
end | ||
|
||
describe '#page_title' do | ||
it 'is blank by default' do | ||
stub_controller_action('widgets', 'show') | ||
store_translations({}) | ||
|
||
result = helper.page_title | ||
|
||
expect(result).to eq '<title></title>' | ||
end | ||
|
||
it "uses the page's i18n locale when present" do | ||
stub_controller_action('widgets', 'show') | ||
store_translations(widgets: { show: { page_title: 'Widgets are fun' } }) | ||
|
||
result = helper.page_title | ||
|
||
expect(result).to eq '<title>Widgets are fun</title>' | ||
end | ||
end | ||
|
||
describe '#page_description' do | ||
it 'is blank by default' do | ||
stub_controller_action('widgets', 'show') | ||
store_translations({}) | ||
|
||
result = helper.page_description | ||
|
||
expect(result).to eq '<meta content="" name="description">' | ||
end | ||
|
||
it "uses the page's i18n locale when present" do | ||
stub_controller_action('widgets', 'show') | ||
store_translations( | ||
widgets: { show: { page_description: 'Widgets are fun' } } | ||
) | ||
|
||
result = helper.page_description | ||
|
||
expect(result).to eq '<meta content="Widgets are fun" name="description">' | ||
end | ||
end | ||
end |
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.
I'd have to look at the HTML spec, but I believe that the
title
element cannot be empty.I realize the intention of flutie is to have a reasonable default and assume that the user would override the default, but it seems weird to generate invalid html by default.
That was part of the motivation of the previous impl using the Rails app name (which, granted, is usually not a good page title, but was the least bad thing I could think of).