Skip to content

Focus on title when creating a new post.#599

Merged
diegoreymendez merged 10 commits intodevelopfrom
try/forward-ref-to-post-title
Feb 15, 2019
Merged

Focus on title when creating a new post.#599
diegoreymendez merged 10 commits intodevelopfrom
try/forward-ref-to-post-title

Conversation

@diegoreymendez
Copy link
Copy Markdown
Contributor

@diegoreymendez diegoreymendez commented Feb 14, 2019 •
edited
Loading

Description:

Resolves wordpress-mobile/WordPress-iOS#10594

This PR focuses the title when opening Gutenberg, if the post has no title and no content. This is a simplified and initial approach, that seems to be enough to match the condition that we're dealing with a new post.

If we find this condition isn't enough we can expand incrementally.

Related PRs:

Gutenberg: WordPress/gutenberg#13874

Testing:

Test 1:

  1. Run the demo app.

Make sure the focus is NOT in the title, since it's not empty and there are blocks.

Test 2:

  1. Edit App.js, and force initialTitle = ''; and initialData = '';.
  2. Run the demo app.
  3. Make sure the title and content are empty. If not, go back to step 1.

Make sur the focus is in the title, since the title is empty and there are no blocks.

@diegoreymendez diegoreymendez added this to the Beta milestone Feb 14, 2019
@diegoreymendez diegoreymendez self-assigned this Feb 14, 2019
@diegoreymendez diegoreymendez mentioned this pull request Feb 14, 2019
Merged
5 tasks
@diegoreymendez diegoreymendez added [Type] Enhancement Improves a current area of the editor and removed [Status] Not Ready For Review labels Feb 14, 2019
daniloercoli
daniloercoli reviewed Feb 14, 2019
View reviewed changes
daniloercoli
daniloercoli reviewed Feb 14, 2019
View reviewed changes
@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Feb 14, 2019

I've left just a couple of comments, but this PR is LGTM once those and GB conflicts are resolved!

Did you test this PR by integrating it into the main wp-ios or wp-android apps?

As discussed in Slack convo, we may want to iterate on this, and read the isNewPost parameter from the parent app (via the bridge/glue code) instead of manually checking it with this.props.title === '' && this.props.blockCount === 0

@diegoreymendez
Copy link
Copy Markdown
Contributor Author

diegoreymendez commented Feb 15, 2019 •
edited
Loading

As discussed in Slack convo, we may want to iterate on this, and read the isNewPost parameter from the parent app (via the bridge/glue code) instead of manually checking it with this.props.title === '' && this.props.blockCount === 0

No, but I purposely left that out of the scope of this PR. As soon as we close this I'll submit a PR to test the integration and make any necessary changes (which I expect will be none).

In my modest opinion, it's in our best interest to try and work with more and more incrementally, unless there's a good reason to hold everything back.

This ensures we don't spend a lot of time synching both open PRs due to very small pending changes / fixes.

We've seen some of this in the past and I'd like to avoid spending time to maintain a lot of open PRs unless it's really necessary to avoid breakage (in this case, the result won't be worse than it was before).

@diegoreymendez
Copy link
Copy Markdown
Contributor Author

diegoreymendez commented Feb 15, 2019 •
edited
Loading

Comments adressed @daniloercoli .

daniloercoli
daniloercoli approved these changes Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

LGTM once gutenberg ref is updated.

@diegoreymendez diegoreymendez merged commit ac16ff3 into develop Feb 15, 2019
@diegoreymendez diegoreymendez deleted the try/forward-ref-to-post-title branch February 15, 2019 15:17
@daniloercoli daniloercoli mentioned this pull request Feb 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@daniloercoli daniloercoli daniloercoli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@diegoreymendez diegoreymendez

Labels

[Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

Beta

Development

Successfully merging this pull request may close these issues.

3 participants

@diegoreymendez @daniloercoli @Tug