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

Focus on title when creating a new post. #599

Merged
merged 10 commits into from
Feb 15, 2019

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Feb 14, 2019

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 [Type] Enhancement Improves a current area of the editor and removed [Status] Not Ready For Review labels Feb 14, 2019
src/app/App.js Outdated Show resolved Hide resolved
@daniloercoli
Copy link
Contributor

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
Contributor Author

diegoreymendez commented Feb 15, 2019

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
Contributor Author

diegoreymendez commented Feb 15, 2019

Comments adressed @daniloercoli .

Copy link
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants