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

Update document.title when post title changes #1802

Merged
merged 10 commits into from
Jul 20, 2017
46 changes: 46 additions & 0 deletions editor/document-title/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* External dependencies
*/
import { connect } from 'react-redux';
import { Component } from 'react';

/**
* Internal dependencies
*/
import { getDocumentTitle } from '../selectors';

class DocumentTitle extends Component {

constructor( props ) {
super( props );
this.originalDocumentTitle = document.title;
}

setDocumentTitle( title ) {
document.title = title + ' | ' + this.originalDocumentTitle;
}

componentDidMount() {
this.setDocumentTitle( this.props.title );
}

componentWillReceiveProps( nextProps ) {
if ( nextProps.title !== this.props.title ) {
this.setDocumentTitle( nextProps.title );
}
}

render() {
Copy link
Member

Choose a reason for hiding this comment

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

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

  • Constructor
  • Lifecycle
  • Helper methods
  • Render

return null;
}

componentWillUnmount() {
document.title = this.originalDocumentTitle;
}
}

export default connect(
( state ) => ( {
title: getDocumentTitle( state ),
} )
)( DocumentTitle );
4 changes: 3 additions & 1 deletion editor/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { connect } from 'react-redux';
import classnames from 'classnames';

/**
* WordPress Dependencie
* WordPress dependencies
*/
import { NoticeList } from 'components';

Expand All @@ -18,6 +18,7 @@ import Sidebar from '../sidebar';
import TextEditor from '../modes/text-editor';
import VisualEditor from '../modes/visual-editor';
import UnsavedChangesWarning from '../unsaved-changes-warning';
import DocumentTitle from '../document-title';
import { removeNotice } from '../actions';
import {
getEditorMode,
Expand All @@ -32,6 +33,7 @@ function Layout( { mode, isSidebarOpened, notices, ...props } ) {

return (
<div className={ className }>
<DocumentTitle />
<NoticeList onRemove={ props.removeNotice } notices={ notices } />
<UnsavedChangesWarning />
<Header />
Expand Down
39 changes: 36 additions & 3 deletions editor/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import { getBlockType } from 'blocks';
*/
import { addQueryArgs } from './utils/url';

/**
* WordPress dependencies
*/
import { __ } from 'i18n';

/**
* Returns the current editing mode.
*
Expand Down Expand Up @@ -88,6 +93,17 @@ export function isEditedPostDirty( state ) {
return state.editor.dirty;
}

/**
* Returns true if there are no unsaved values for the current edit session and if
* the currently edited post is new (and has never been saved before).
*
* @param {Object} state Global application state
* @return {Boolean} Whether new post and unsaved values exist
*/
export function isCleanNewPost( state ) {
return ! isEditedPostDirty( state ) && isEditedPostNew( state );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a unit test for this selector?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Done in 1f6601a8568261ec92f8dabc8d2b853895e9abdc.


/**
* Returns the post currently being edited in its last known saved state, not
* including unsaved edits. Returns an object containing relevant default post
Expand Down Expand Up @@ -226,9 +242,26 @@ export function isEditedPostBeingScheduled( state ) {
* @return {String} Raw post title
*/
export function getEditedPostTitle( state ) {
return state.editor.edits.title === undefined
? get( state.currentPost, 'title.raw' )
: state.editor.edits.title;
const editedTitle = get( state.editor, 'edits.title' );
Copy link
Member

Choose a reason for hiding this comment

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

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

if ( editedTitle !== undefined ) {
return editedTitle;
}
return get( state.currentPost, 'title.raw' );
}

/**
* Gets the document title to be used.
*
* @param {Object} state Global application state
* @return {string} Document title
*/
export function getDocumentTitle( state ) {
let title = getEditedPostTitle( state );

if ( ! title || '' === title.trim() ) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

title = isCleanNewPost( state ) ? __( 'New post' ) : __( '(Untitled)' );
}
return title;
}

/**
Expand Down
13 changes: 11 additions & 2 deletions editor/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import optimist from 'redux-optimist';
import { combineReducers, applyMiddleware, createStore } from 'redux';
import refx from 'refx';
import { reduce, keyBy, first, last, omit, without, flowRight } from 'lodash';
import { reduce, keyBy, first, last, omit, without, flowRight, forOwn } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -18,6 +18,7 @@ import { combineUndoableReducers } from './utils/undoable-reducer';
import effects from './effects';

const isMobile = window.innerWidth < 782;
const renderedPostProps = new Set( [ 'guid', 'title', 'excerpt', 'content' ] );

/**
* Undoable reducer returning the editor post state, including blocks parsed
Expand Down Expand Up @@ -263,7 +264,15 @@ export function currentPost( state = {}, action ) {
return action.post;

case 'UPDATE_POST':
return { ...state, ...action.edits };
const post = { ...state };
forOwn( action.edits, ( value, key ) => {
if ( renderedPostProps.has( key ) ) {
post[ key ] = { raw: value };
} else {
post[ key ] = value;
}
} );
return post;
}

return state;
Expand Down
114 changes: 114 additions & 0 deletions editor/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ import {
hasEditorRedo,
isEditedPostNew,
isEditedPostDirty,
isCleanNewPost,
getCurrentPost,
getCurrentPostId,
getCurrentPostType,
getPostEdits,
getEditedPostTitle,
getDocumentTitle,
getEditedPostExcerpt,
getEditedPostVisibility,
isEditedPostPublished,
Expand Down Expand Up @@ -53,6 +55,11 @@ import {
getNotices,
} from '../selectors';

/**
* WordPress dependencies
*/
import { __ } from 'i18n';

describe( 'selectors', () => {
describe( 'getEditorMode', () => {
it( 'should return the selected editor mode', () => {
Expand Down Expand Up @@ -174,12 +181,48 @@ describe( 'selectors', () => {
editor: {
dirty: false,
},
currentPost: {},
};

expect( isEditedPostDirty( state ) ).toBe( false );
} );
} );

describe( 'isCleanNewPost', () => {
it( 'should return true when the post is not dirty and has not been saved before', () => {
const state = {
editor: {
dirty: false,
},
currentPost: {},
};

expect( isCleanNewPost( state ) ).toBe( true );
} );

it( 'should return false when the post is not dirty but the post has been saved', () => {
const state = {
editor: {
dirty: false,
},
currentPost: { id: 1 },
};

expect( isCleanNewPost( state ) ).toBe( false );
} );

it( 'should return false when the post is dirty but the post has not been saved', () => {
const state = {
editor: {
dirty: true,
currentPost: {},
},
};

expect( isCleanNewPost( state ) ).toBe( false );
} );
} );

describe( 'getCurrentPost', () => {
it( 'should return the current post', () => {
const state = {
Expand Down Expand Up @@ -260,6 +303,77 @@ describe( 'selectors', () => {
} );
} );

describe( 'getDocumentTitle', () => {
it( 'should return current title unedited existing post', () => {
const state = {
currentPost: {
id: 123,
title: { raw: 'The Title' },
},
editor: {
dirty: false,
},
};

expect( getDocumentTitle( state ) ).toBe( 'The Title' );
} );

it( 'should return current title for edited existing post', () => {
const state = {
currentPost: {
id: 123,
title: { raw: 'The Title' },
},
editor: {
dirty: true,
edits: { title: 'Modified Title' },
},
};

expect( getDocumentTitle( state ) ).toBe( 'Modified Title' );
} );

it( 'should return new post title when new post is clean', () => {
const state = {
currentPost: {
title: { raw: '' },
},
editor: {
dirty: false,
},
};

expect( getDocumentTitle( state ) ).toBe( __( 'New post' ) );
} );

it( 'should return untitled title when new post is dirty', () => {
const state = {
currentPost: {
title: { raw: '' },
},
editor: {
dirty: true,
},
};

expect( getDocumentTitle( state ) ).toBe( __( '(Untitled)' ) );
} );

it( 'should return untitled title', () => {
const state = {
currentPost: {
id: 123,
title: { raw: '' },
},
editor: {
dirty: true,
},
};

expect( getDocumentTitle( state ) ).toBe( __( '(Untitled)' ) );
} );
} );

describe( 'getEditedPostExcerpt', () => {
it( 'should return the post saved excerpt if the excerpt is not edited', () => {
const state = {
Expand Down
10 changes: 5 additions & 5 deletions editor/test/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -603,22 +603,22 @@ describe( 'state', () => {

describe( 'currentPost()', () => {
it( 'should reset a post object', () => {
const original = deepFreeze( { title: 'unmodified' } );
const original = deepFreeze( { title: { raw: 'unmodified' } } );

const state = currentPost( original, {
type: 'RESET_POST',
post: {
title: 'new post',
title: { raw: 'new post' },
},
} );

expect( state ).toEqual( {
title: 'new post',
title: { raw: 'new post' },
} );
} );

it( 'should update the post object with UPDATE_POST', () => {
const original = deepFreeze( { title: 'unmodified', status: 'publish' } );
const original = deepFreeze( { title: { raw: 'unmodified' }, status: 'publish' } );

const state = currentPost( original, {
type: 'UPDATE_POST',
Expand All @@ -628,7 +628,7 @@ describe( 'state', () => {
} );

expect( state ).toEqual( {
title: 'updated post object from server',
title: { raw: 'updated post object from server' },
status: 'publish',
} );
} );
Expand Down
2 changes: 1 addition & 1 deletion lib/register.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function gutenberg_menu() {

add_submenu_page(
'gutenberg',
__( 'New Post', 'gutenberg' ),
__( 'Gutenberg', 'gutenberg' ),
__( 'New Post', 'gutenberg' ),
'edit_posts',
'gutenberg',
Expand Down