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 defaultTitle as fallback when title is not defined #117

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

jmurzy
Copy link
Contributor

@jmurzy jmurzy commented Mar 17, 2016

Adds a defaultTitle option to be used as fallback when a title is not defined.

<Helmet defaultTitle="Aviato" titleTemplate="%s | Aviato"  />

yields

<title>Aviato</title>

and,

<Helmet Title="Contact" />

when composed as a child, yields

<title>Contact | Aviato</title>

Fixes #109.

Let me know if this is something you'd consider merging, and I'll sign the CLA.

🍺

@jmurzy
Copy link
Contributor Author

jmurzy commented Mar 17, 2016

Please note that the build is currently failing due to estools/escope#99 and babel/babel-eslint#243.

@cwelch5 @doctyper

@@ -24,7 +24,7 @@ describe("Helmet", () => {
describe("title", () => {
it("can update page title", () => {
ReactDOM.render(
<Helmet title={"Test Title"} />,
<Helmet title={"Test Title"} defaultTitle={"Fallback"} />,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, nitpick, but if you could please put these props on new lines, like your other test case below, that would be great. Otherwise, everything looks perfect.

@cwelch5
Copy link
Contributor

cwelch5 commented Mar 18, 2016

Thx @jmurzy, I made one line note, but it looks like a good solution.

@jmurzy jmurzy force-pushed the jmurzy-defaultTitle branch from e211f6b to 843a320 Compare March 18, 2016 22:47
@jmurzy
Copy link
Contributor Author

jmurzy commented Mar 18, 2016

@cwelch5 Fixed. Thanks!

🍺

@doctyper
Copy link
Contributor

@jmurzy Merging master in should fix the failing build.

@jmurzy
Copy link
Contributor Author

jmurzy commented Mar 24, 2016

@doctyper Done. Thanks.

🍺

@jmurzy jmurzy force-pushed the jmurzy-defaultTitle branch from be8fb1a to 372a027 Compare March 24, 2016 19:20
@cwelch5 cwelch5 merged commit 8e81d51 into nfl:master Mar 24, 2016
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.

3 participants