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

Editor: Always focus title field on new draft #1696

Merged
merged 8 commits into from
Jan 5, 2016
Merged

Conversation

timmyc
Copy link
Contributor

@timmyc timmyc commented Dec 16, 2015

Fixes #1622

When you first open the editor via the pencil or add in a site sidebar, the title field is focused and ready to receive a witty post title. Once within the editor though, any action that results in another new post - the title field is not auto focused. This branch aims to focus the title field on all new drafts - regardless of how one arrives at that new draft ( only on non-mobile viewports ).

To Test

  • Open up the editor via the pencil in the masterbar
  • Note the title field will be focused on a desktop viewport sized browser
  • Expand your draft drawer, load in a draft post - or load an existing published post
  • Note the title field does not focus
  • Now click the pencil again, select a site if applicable
  • Once the new post loads, note that the title field is again focused

@timmyc timmyc added the [Feature] Post/Page Editor The editor for editing posts and pages. label Dec 16, 2015
@timmyc timmyc self-assigned this Dec 16, 2015
@@ -4,6 +4,7 @@
import React, { PropTypes } from 'react';
import classNames from 'classnames';
import omit from 'lodash/object/omit';
import ReactDOM from 'react-dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling this will become an ongoing battle ( 😄 ), but per our naming conventions, this should be ReactDom (reference).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may or may not have done this on purpose :trollface: - well it is what the deprecation notice told me to use in the console 😆

@timmyc
Copy link
Contributor Author

timmyc commented Dec 16, 2015

@aduth added focus() to <FormTextInput/> in cc8981c - also took the opportunity to cleanup the component a bit, and used ReactDom.

@lancewillett lancewillett added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 16, 2015
'is-valid': this.props.isValid
} );
focus() {
ReactDom.findDOMNode( this.refs.textField ).focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Part of the benefit of moving focus to <FormTextInput /> is that now you have access to the "raw" DOM element, so you no longer need findDOMNode. Here, this.refs.textField is the a reference to the DOM node itself, so this can simply be this.refs.textField.focus() and you can drop the react-dom dependency.

@aduth 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 Dec 17, 2015
@timmyc timmyc 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 Dec 17, 2015
@timmyc
Copy link
Contributor Author

timmyc commented Dec 17, 2015

Thanks @aduth - updated.

},

render() {
const otherProps = omit( this.props, [ 'className', 'type', 'ref' ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could have left ref out, since I don't think ref is passed as a prop even when assigned in a parent component, and because you override it after applying the spread properties to the <input > element. In fact, this whole omit statement could be left out altogether -

<input { ...this.props } ref="textField" type={ type } className={ classes } />

It's a small detail, so not to hold up merge.

@aduth
Copy link
Contributor

aduth commented Dec 18, 2015

Minor details be minor details. This works well in my testing 👍

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 18, 2015
@lancewillett
Copy link
Contributor

Needs refresh.

@timmyc timmyc 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 Jan 1, 2016
@kellychoffman
Copy link
Member

👍 Nice. Looks good.

@kellychoffman kellychoffman added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 4, 2016
timmyc added a commit that referenced this pull request Jan 5, 2016
Editor: Always focus title field on new draft
@timmyc timmyc merged commit b891638 into master Jan 5, 2016
@lancewillett lancewillett deleted the fix/editor/1622 branch January 14, 2016 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants