Update document.title when post title changes #1802

westonruter · 2017-07-07T23:45:11Z

When the page is loading, the tab looks like:

The post title is then prepended to the original title that was output by PHP.

When clicking “New Post” link the tab looks like:

Upon typing “Initial” the tab is live updated to:

If the title is then cleared out, on a new post or an existing post, then the tab appears as:

Fixes #1737.

youknowriad · 2017-07-08T08:10:46Z

editor/post-title/index.js

@@ -119,6 +119,7 @@ export default connect(
 return {
 onUpdate( title ) {
 dispatch( editPost( { title } ) );
+ dispatch( editPostTitle( title ) );


Could we just rely on the editPost action instead and this beeing a side-effect if the title is in the edits maybe?

Good point. Done in 1dcd6f880af805344f503c1b1f2e9bdd604cbe63.

youknowriad · 2017-07-08T08:12:42Z

editor/selectors.js

@@ -74,6 +74,17 @@ export function isEditedPostDirty( state ) {
 }

 /**
+ * Returns true if there are unsaved values for the current edit session and if


I think this function does the opposite right?

Yes. Done in 1f6601a8568261ec92f8dabc8d2b853895e9abdc.

youknowriad · 2017-07-08T08:13:20Z

editor/selectors.js

+ */
+export function isCleanNewPost( state ) {
+ return ! isEditedPostDirty( state ) && isEditedPostNew( state );
+}


Should we add a unit test for this selector?

Yes. Done in 1f6601a8568261ec92f8dabc8d2b853895e9abdc.

coveralls · 2017-07-09T02:52:26Z

Changes Unknown when pulling 1f6601a8568261ec92f8dabc8d2b853895e9abdc on add/document-title into ** on master**.

aduth · 2017-07-10T10:33:12Z

lib/register.php

@@ -44,7 +44,7 @@ function gutenberg_menu() {

 add_submenu_page(
 'gutenberg',
- __( 'New Post', 'gutenberg' ),
+ __( 'Gutenberg ', 'gutenberg' ),


Trailing space intentional?

aduth · 2017-07-10T10:33:40Z

editor/effects.js

+let originalDocumentTitle;
+
+function populateDocumentTitle( title, isCleanNew = false ) {
+ if ( ! title.trim() ) {


Seems like some of this logic makes more sense in a selector, like getDocumentTitle.

Done in 8cec2c9.

aduth · 2017-07-10T10:34:28Z

editor/effects.js

+ if ( ! title.trim() ) {
+ title = isCleanNew ? __( 'New post' ) : __( '(Untitled)' );
+ }
+ if ( ! originalDocumentTitle ) {


Instead of tracking this as inferred from the original title, should not the client be able to generate the desired title at any point in time, including when the editor first initializes?

Potentially. However, that would require that we export a template from PHP that we can use in JS. This was a quicker way to achieve that goal.

The Customizer implements this via:

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/class-wp-customize-manager.php#L3630

https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/js/customize-controls.js#L3646-L3659

I don't feel it is entirely clear where we should be exporting data from PHP to JS generally.

aduth · 2017-07-10T10:35:32Z

editor/effects.js

+let originalDocumentTitle;
+
+function populateDocumentTitle( title, isCleanNew = false ) {
+ if ( ! title.trim() ) {


Should we be using the trimmed title when assigning document.title ?

It's unnecessary because document.title has its whitespace automatically normalized: https://html.spec.whatwg.org/multipage/dom.html#document.title

westonruter · 2017-07-10T23:33:55Z

editor/effects.js

 export default {
+ RESET_POST( action, store ) {
+ setTimeout( () => { // Next-tick to ensure action has been applied to state in store .


@aduth This next-tick feels hacky, but I didn't see a reliable way to access the store with the action applied to it without doing so.

Related: #1816 (comment)

An alternative here is to not treat these as effects of the action, but rather handled by logic which has subscribed to store updates (e.g. a connected component, or a store.subscribe callback).

This is the approach we had taken in the Calypso editor. Basically: Instead of binding to the specific actions that we think would cause a change to the document title selector value, we instead re-run the selector any time any dispatch occurs, and if the result is different, apply the new title value.

@aduth check out the latest changes. It feels much better. I looked at the DocumentHead component in Calypso and used it to inspire a DocumentTitle component.

westonruter · 2017-07-11T02:53:16Z

Rebased from 8cec2c9d and amended tests to use Jest API.

westonruter · 2017-07-19T06:27:03Z

Rebasing. Former HEAD was 3efc5b8.

…schema in UPDATE_POST action

aduth

Seems to work well in my testing. I left a few minor comments to address if you wish.

Only other thought I have is: Do we really want the document title to be assigned as the post's title? Per #1737 and current editor behavior, seems "Edit Post" could work just as well. Was there any conversation or thoughts behind the choice to use post title?

aduth · 2017-07-20T19:53:09Z

editor/document-title/index.js

+ }
+ }
+
+ render() {


Minor: I think we should stick with convention to have render be last method in component, roughly something like:

Constructor

Lifecycle

Helper methods

Render

aduth · 2017-07-20T20:00:35Z

editor/selectors.js

- return state.editor.edits.title === undefined
- ? get( state.currentPost, 'title.raw' )
- : state.editor.edits.title;
+ const editedTitle = get( state.editor, 'edits.title' );


A few minor thoughts:

get with string as the second argument is significantly slower than array alternative (benchmark)

state.editor.edits is always an object (based on reducer default value) and we don't need get to test its truthiness, for a trivial performance improvement: state.editor.edits.title

We should compose selectors vs. accessing state directly when possible, i.e. getPostEdits instead of state.editor.edits, getCurrentPost instead of state.currentPost

get accepts a third argument for default value, so this could be expressed as a single return value, though maybe not as readable

aduth · 2017-07-20T20:02:56Z

editor/selectors.js

+export function getDocumentTitle( state ) {
+ let title = getEditedPostTitle( state );
+
+ if ( ! title || '' === title.trim() ) {


Maybe getEditedPostTitle should return an empty string as its default? In which case we could consolidate this as if ( ! title.trim() ) { . Not sure if there's a performance benefit to testing truthiness before a trim, but readability benefit in testing in single pass.

westonruter · 2017-07-20T22:38:14Z

@aduth:

Only other thought I have is: Do we really want the document title to be assigned as the post's title? Per #1737 and current editor behavior, seems "Edit Post" could work just as well. Was there any conversation or thoughts behind the choice to use post title?

Good point. In fact, I just assumed that “new post” would be replaced with the post title. I think I instinctively wanted to include the post title in the title because when you have multiple posts open in separate tabs, it is very annoying to easily identify the tab for the post you want to edit. Case in point with 4 tabs open for separate posts:

So this PR fixes that usability issue.

westonruter · 2017-07-20T22:38:52Z

I'll fix up your other issues you pointed out. Thank you for reviewing.

westonruter requested a review from nylen July 7, 2017 23:45

youknowriad reviewed Jul 8, 2017

View reviewed changes

aduth reviewed Jul 10, 2017

View reviewed changes

westonruter commented Jul 10, 2017

View reviewed changes

westonruter force-pushed the add/document-title branch from 8cec2c9 to 3efc5b8 Compare July 11, 2017 02:52

westonruter added 7 commits July 18, 2017 22:33

Update document.title when post title changes

98303bb

Eliminate EDIT_POST_TITLE action in favor of reusing EDIT_POST

b406fd2

Add tests for isCleanNewPost selector; fix description

0a18639

Add getDocumentTitle selector

c428f3a

Update tests to use Jest API

e0c47b8

Add DocumentTitle component

37873da

Store original document title on DocumentTitle; restore when unmounted

b142f8b

westonruter force-pushed the add/document-title branch from 3efc5b8 to b142f8b Compare July 19, 2017 06:27

westonruter added 2 commits July 19, 2017 00:23

Remove side effects from render function

f4145f3

Ensure currentPost reducer wraps edits in {raw} objects according to …

7462db6

…schema in UPDATE_POST action

aduth approved these changes Jul 20, 2017

View reviewed changes

Normalize component method ordering and selector usage

2309add

westonruter merged commit a23904b into master Jul 20, 2017

westonruter deleted the add/document-title branch July 20, 2017 23:04

mtias mentioned this pull request Nov 29, 2017

Restore existing behavior when rendering document title (<title> tag) #3702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update document.title when post title changes #1802

Update document.title when post title changes #1802

westonruter commented Jul 7, 2017

youknowriad Jul 8, 2017

westonruter Jul 9, 2017

youknowriad Jul 8, 2017

westonruter Jul 9, 2017

youknowriad Jul 8, 2017

westonruter Jul 9, 2017

coveralls commented Jul 9, 2017

aduth Jul 10, 2017

aduth Jul 10, 2017

westonruter Jul 10, 2017

aduth Jul 10, 2017

westonruter Jul 10, 2017

aduth Jul 10, 2017

westonruter Jul 10, 2017

westonruter Jul 10, 2017

aduth Jul 13, 2017

aduth Jul 13, 2017

westonruter Jul 19, 2017

westonruter commented Jul 11, 2017

westonruter commented Jul 19, 2017

aduth left a comment

aduth Jul 20, 2017

aduth Jul 20, 2017

aduth Jul 20, 2017

westonruter commented Jul 20, 2017

westonruter commented Jul 20, 2017

Update document.title when post title changes #1802

Update document.title when post title changes #1802

Conversation

westonruter commented Jul 7, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 9, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jul 11, 2017

westonruter commented Jul 19, 2017

aduth left a comment

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

westonruter commented Jul 20, 2017

westonruter commented Jul 20, 2017