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

Remove JS handling of title focus #633

Merged
merged 1 commit into from
Feb 21, 2019
Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Feb 20, 2019

There is a bridge message in place to let the parent app tell gutenberg when it should focus the title. Android seems to be using that mechanism to focus the title.
(Is that correct @daniloercoli ?)

This PR implement the title focus feature using this message from the parent app instead of letting the JS side do it by itself.

The main reason of this change is a glitch on the toolbar + SafeAreas.
Check the WPiOS related PR to see the native side implementation.

Before:
focus-bug

Now:
focus-fix

To test:

  • Check out WPAndroid with this branch, and check that the autofocus on new posts still works.
  • Checkout the WPiOS related PR running metro on this branch.
  • Check that the autofocus on new posts works properly.
  • Check that the toolbar is properly positioned.

cc @pinarol - This might also fix the title border issue on the initial autofocus mentioned here: #622 (review)

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Looks and works all good! 🎉 The toolbar layout seems to get fixed.
I've checked both iOS and Android and title is focused properly in an empty post. But I say let's wait for @daniloercoli 's approval for Android also.

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.

I'm ok with these changes, I've just a small note to add.
The demo app doesn't set the focus on the title if both the content and the title are set empty in initial content. Anyway it doesn't seem to be a big problem at the moment.

@etoledom
Copy link
Contributor Author

@pinarol @daniloercoli - Thank you!

The demo app doesn't set the focus on the title if both the content and the title are set empty in initial content

That's because the parent app has to send the message. I believe that it's a good thing, since it adds flexibility.

@etoledom etoledom merged commit a2c18b8 into develop Feb 21, 2019
@etoledom etoledom deleted the issue/ios-title-focus-toolbar branch February 21, 2019 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants