changed title for better seo #2471
Conversation
| @@ -418,7 +416,7 @@ class Collection extends React.Component { | |||
| > | |||
| <article className="collection"> | |||
| <Helmet> | |||
| <title>{this.getTitle()}</title> | |||
| <title>{this.getTitle() + title}</title> | |||
There was a problem hiding this comment.
The title variable will be null when the collection is still loading, and in that situation I think this will render "p5.js Web Editor | null"? Because the null is cast to a string when using the + operator.
There was a problem hiding this comment.
Oh, Okay i though same and checked that, in almost all cases it was correct but still that might be a case .
So i think we might just render Your Collection and Someone Collection in case of null. To avoid title be null.
There was a problem hiding this comment.
Is this sound good to you?
| const collectionTitle = this.props.t('Collection.Title'); | ||
| const titleSplits = collectionTitle.split('|'); | ||
| const title = titleSplits[0].concat('| '); | ||
| return title; |
There was a problem hiding this comment.
We're basically just extracting the "p5.js Web Editor" portion of the title, right? We don't have any property in the translations files which is just "p5.js Web Editor" but I've wondered before if we should add one. Like t('Common.SiteName') or something. FYI.
There was a problem hiding this comment.
Oh, i was thinking same. Even searched for entire i8n for it. But though i might need to discuss that before putting in. So i stick with the safe option.
Okay now i will implement it.
|
@lindapaiste, Hey i have made changes according to your suggestion. |
# Conflicts: # client/modules/User/components/Collection.jsx
|
Hey, sorry for not keeping on top of this! I was working on something else today which would benefit from your There's a bit of a problem with the title in the loading case because This PR is 95% there so I'm going to go ahead and fix it so that we can get it merged. |
|
Hey, Thanks for letting me about the problem. It was working properly last time when I checked, but I suppose your commit will take care of this problem. |
Fixes #2251
Changes:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123