SSR: Use document-head to populate `title`, `meta`s, and `link`s; drop `Helmet` #7600

ockham · 2016-08-22T23:19:56Z

This PR:

has the theme sheet and showcase use DocumentHead instead of Head (which was a rather specialized component tailored to the theme showcase's SEO needs, based on `react-helmet``
drops Head, and the react-helmet dependency
modifies server/render to use data from the document-head state to populate SEO relevant fields (title, meta, link) for SSR
removes description related reducer and actions.
- Description meta can be handled by just using addMeta, so I don't think it's worthwhile having specialized functions around.

To test:

While logged out, check a given theme sheet's page source. Verify that SEO relevant tags are present (title, meta, link). (Compare with master or production.)
While logged out, navigate to the showcase (http://calypso.localhost:3000/design), and click on any theme. Watch the title change -- at no point should it not make any sense.
Same when logged in. (Also, there obviously shouldn't be any errors, console or otherwise.)

TODO (separate PR):

Make sure we SSR title et al for /design (this isn't currently done on master, so not a regression)

Fixes #3795

Test live: https://calypso.live/?branch=remove/head

aduth · 2016-08-25T03:03:51Z

client/my-sites/theme/main.jsx

@@ -348,46 +348,58 @@ const ThemeSheet = React.createClass( {
 		const analyticsPageTitle = `Themes > Details Sheet${ section ? ' > ' + titlecase( section ) : '' }${ siteID ? ' > Site' : '' }`;

 		const { name: themeName, description } = this.props;
-		const title = i18n.translate( '%(themeName)s Theme', {
+		const title = themeName && i18n.translate( '%(themeName)s Theme', {


What was the reason for the themeName && change? This won't prevent the <DocumentHead /> from dispatching the title, will it?

Per:

wp-calypso/client/components/data/document-head/index.jsx

Lines 29 to 31 in 9923003

if ( 'title' in this.props ) {

this.props.setTitle( title );

}

What was the reason for the themeName && change?

Basically to retain logic that was previously here.

aduth · 2016-08-25T03:09:26Z

Looking forward to this 🎉

aduth · 2016-09-28T16:20:53Z

client/my-sites/themes/theme-showcase.jsx

 		return (
 			<Main className="themes">
+				<DocumentHead title={ themesMeta[ tier ].title } metas={ metas } />


The prop for metas is meta, not metas.

aduth · 2016-09-28T16:22:41Z

client/state/document-head/selectors.js

+ * @param  {Object}  state  Global state tree
+ * @return {Object[]}       Array of meta objects
+ */
+export function getMeta( state ) {


Hmm, in the context of global selectors, should they come to reality, would getMeta be sufficiently unique? Seems less so than getTitle, though a similar argument could be made there.

Okay, I'll rename them to be in line with the setters (i.e. actions).

aduth · 2016-09-28T16:25:31Z

server/render/index.js

+			const title = getFormattedTitle( context.store.getState() );
+			const metas = getMeta( context.store.getState() );
+			const links = getLink( context.store.getState() );
+			context.head = { title, metas, links };


We wrap this in context.store condition, but the Jade template assumes head.title will always exist. Do we need to be safer there too? Or remove the context.store check?

We can't just remove the context.store check or context.store.getState() might fail. Fixing this with f7ffb39 by falling back to undefined attrs.

ockham · 2016-09-28T20:38:06Z

Okay, addressed feedback, including renaming of all selectors. Some have pretty long names now...

aduth · 2016-09-29T13:17:53Z

client/state/document-head/selectors.js

@@ -19,7 +21,7 @@ const UNREAD_COUNT_CAP = 40;
 * @param  {Object}  state  Global state tree
 * @return {?String}        Document title
 */
-export function getTitle( state ) {
+export function getDocumentHeadTitle( state ) {


client/reader/full-post/controller.js still imports as getTitle in this branch.

aduth · 2016-09-29T13:23:12Z

server/render/index.js

+			context.initialReduxState = pick( context.store.getState(), 'documentHead', 'ui', 'themes' );
+		}
+
+		context.head = { title, metas, links };


Do the defaults and context.head assignment need to be outside of the config / context.user condition block too to ensure it's always set?

Good call. This might've gone unnoticed in testing since the feature flag is on in all environments, and context.user is notoriously unset on the server in development.

Description meta can be handled by just using addMeta, so I don't think it's worthwhile having specialized functions around.

…[Capped]UnreadCount

…ttedTitle

ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress Server Side Rendering labels Aug 22, 2016

ockham self-assigned this Aug 22, 2016

ockham force-pushed the remove/head branch from c9e6df2 to 5b98b67 Compare August 23, 2016 11:30

ockham mentioned this pull request Aug 23, 2016

Themes: Make makeElement() helper a themes-agnostic HoF #3425

Closed

4 tasks

aduth reviewed Aug 25, 2016
View reviewed changes

ockham force-pushed the remove/head branch from 5b98b67 to 0d8a1af Compare August 30, 2016 20:37

ockham force-pushed the remove/head branch 5 times, most recently from 9407ab7 to 8bccbf2 Compare September 27, 2016 00:40

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Sep 27, 2016

ockham force-pushed the remove/head branch 2 times, most recently from 033a4b0 to 0a7221d Compare September 28, 2016 13:57

aduth reviewed Sep 28, 2016

View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 28, 2016

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Sep 28, 2016

aduth suggested changes Sep 29, 2016

View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 29, 2016

ockham force-pushed the remove/head branch from 587bee7 to 35d5e9c Compare September 29, 2016 23:33

ockham added 22 commits October 3, 2016 19:36

Head: Remove

8790325

Framework: Remove react-helmet dependency

cb1f878

Themes: Add fallback tier

887e01e

Themes: Wrap a long line

de204d6

Theme Showcase: Remove isRequired from tier prop, as there's a default

9cd1de8

document-head reducer: Set default og:site_name meta to WordPress.com

71968da

document-head state: Add tests for getMeta() and getLink() selectors

0b060ac

SSR: Wrap title, metas, and links attrs in a head object

9f6fc81

document-head state: Remove description related reducer and actions

cba548a

Description meta can be handled by just using addMeta, so I don't think it's worthwhile having specialized functions around.

document-head component: Fix prop checks

5cdcb72

ThemeShowcase: Fix prop name (metas -> meta)

578a391

SSR: Fall back to empty context.head attrs

52f1b8c

state/document-head: Rename get{Meta|Link} to getDocumentHead{Meta|Link}

7137280

state/document-head: Rename getTitle to getDocumentHeadTitle

c613aac

state/document-head: Rename get[Capped]UnreadCount to getDocumentHead…

9c5b75a

…[Capped]UnreadCount

state/document-head: Rename getFormattedTitle to getDocumentHeadForma…

9b9a783

…ttedTitle

SSR: Really fix fallback for metas and links

d8b7e02

reader/full-post/controller: import { getDocumentHeadTitle as getTitle }

4ea8e46

SSR: Move head default so they're unconditionally available

67d0979

SSR: Initialize Redux Store also for non-iso sections

79e9ba2

SSR: ES6ify server/pages

133546e

SSR: Restrict conditions to !== undefined

9f9bf27

seear approved these changes Oct 3, 2016

View reviewed changes

ockham force-pushed the remove/head branch from 20bc184 to 9f9bf27 Compare October 3, 2016 19:24

ockham merged commit cb45a9a into master Oct 3, 2016

ockham deleted the remove/head branch October 3, 2016 19:35

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSR: Use document-head to populate `title`, `meta`s, and `link`s; drop `Helmet` #7600

SSR: Use document-head to populate `title`, `meta`s, and `link`s; drop `Helmet` #7600

ockham commented Aug 22, 2016 •

edited

Loading

aduth Aug 25, 2016

ockham Aug 30, 2016

aduth commented Aug 25, 2016

aduth Sep 28, 2016

aduth Sep 28, 2016 •

edited

Loading

ockham Sep 28, 2016

aduth Sep 28, 2016

ockham Sep 28, 2016

ockham commented Sep 28, 2016

aduth Sep 29, 2016

aduth Sep 29, 2016

ockham Sep 29, 2016

	if ( 'title' in this.props ) {
	this.props.setTitle( title );
	}

SSR: Use document-head to populate title, metas, and links; drop Helmet #7600

SSR: Use document-head to populate title, metas, and links; drop Helmet #7600

Conversation

ockham commented Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Aug 25, 2016

Choose a reason for hiding this comment

aduth Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSR: Use document-head to populate `title`, `meta`s, and `link`s; drop `Helmet` #7600

SSR: Use document-head to populate `title`, `meta`s, and `link`s; drop `Helmet` #7600

ockham commented Aug 22, 2016 •

edited

Loading

aduth Sep 28, 2016 •

edited

Loading