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

Allow including the application name in the title #13

Closed
wants to merge 1 commit into from

Conversation

twe4ked
Copy link

@twe4ked twe4ked commented Nov 7, 2016

Cheers.

@@ -49,6 +54,7 @@
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('name' => 'Caleb')

expect(helper.title(greeting: 'Hello')).to eq('Hello Caleb')
expect(helper.title(greeting: 'Hello', include_application_name: true)).to eq('Hello Caleb - Not Dummy')
Copy link
Collaborator

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.
Line is too long. [108/80]

@@ -40,6 +44,7 @@
allow(helper).to receive_message_chain(:controller, :view_assigns).and_return('name' => 'Caleb')

expect(helper.title).to eq('Caleb')
expect(helper.title(include_application_name: true)).to eq('Caleb - Not Dummy')
Copy link
Collaborator

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.
Line is too long. [83/80]

@@ -31,6 +34,7 @@
load_translations(dashboards: { show: 'Dashboard' })

expect(helper.title).to eq('Dashboard')
expect(helper.title(include_application_name: true)).to eq('Dashboard - Not Dummy')
Copy link
Collaborator

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.
Line is too long. [87/80]

@@ -23,6 +25,7 @@
load_translations(engine: { default: 'Engine name' })

expect(helper.title).to eq 'Engine name'
expect(helper.title(include_application_name: true)).to eq('Engine name')
Copy link
Collaborator

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.

end

it 'uses the application title by default' do
stub_rails
load_translations(application: 'Not Dummy')

expect(helper.title).to eq('Not Dummy')
expect(helper.title(include_application_name: true)).to eq('Not Dummy')
Copy link
Collaborator

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.

it 'is the app name when no translation is present' do
stub_rails

expect(helper.title).to eq('Dummy')
expect(helper.title(include_application_name: true)).to eq('Dummy')
Copy link
Collaborator

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.

end

private

attr_reader :controller_path, :action_name, :context
attr_reader :controller_path, :action_name, :context, :include_application_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [85/80]

[:titles, controller_name, action_name].join('.'),
context.merge(default: defaults)
)
parts << I18n.t(context.merge(default: defaults)) if include_application_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [85/80]

end

class PageTitle
def initialize(controller_path, action_name, context)
def initialize(controller_path, action_name, context, include_application_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [85/80]

context = controller.view_assigns.merge(additional_context).symbolize_keys
PageTitle.new(controller_path, action_name, context).to_s
PageTitle.new(controller_path, action_name, context, include_application_name).to_s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [89/80]

def title(additional_context = {})
include_application_name = additional_context.delete(:include_application_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [85/80]

@@ -1,27 +1,34 @@
module Title
module TitleHelper
SEPARATOR = ' - '.freeze
Copy link
Collaborator

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.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling da5e6dd on twe4ked:include-application-name into * on calebthompson:master*.

@twe4ked
Copy link
Author

twe4ked commented Nov 7, 2016

Fixing the hound errors will make the style inconsistent with the rest of the file. Thoughts?

@wkirby
Copy link

wkirby commented Jan 19, 2017

Is this likely to be merged?

@twe4ked twe4ked closed this Dec 13, 2017
@twe4ked twe4ked deleted the include-application-name branch December 13, 2017 23:58
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

4 participants