-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implements the editor title in RN for Gutenberg. #10794
Conversation
Generated by 🚫 Danger |
Podfile
Outdated
@@ -96,7 +96,7 @@ target 'WordPress' do | |||
## React Native | |||
## ===================== | |||
## | |||
pod 'Gutenberg', :git => 'http://github.com/wordpress-mobile/gutenberg-mobile/', :commit => '704d003adc1f1b76ee0978aa54e05a36fb16371d' | |||
pod 'Gutenberg', :git => 'http://github.com/wordpress-mobile/gutenberg-mobile/', :commit => 'e98f6fcf21cac614192ef189e9514ec7f4a03af4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me if you already know about this, there's a recent agreement about not depending on commit numbers anymore even on develop branch. So I think we'll need to update the commit dependencies to a beta tag before merging our PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, but I'd suggest to publish a tag once the review process is complete to avoid multiple tags per PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was already agreeing on that sorry if I wasn't clear
So I think we'll need to update the commit dependencies to a beta tag before merging our PRs
Anyway after chatting on this, this conversation might even be outdated
@etoledom was this sth you did as a workaround for alpha? (I mean focusing to empty title at start) As far as I remember we need to focus to title at start weather it is empty or not for beta, if so, we can handle all focusing to title issues separately under this issue #10594 (comment) |
We followed @iamthomasbishop's design feedback indications and implemented the focusing on keyboard for new posts only. wordpress-mobile/gutenberg-mobile#357 (comment) I also agree with @diegoreymendez that it would be good to reimplement this behavior on a different PR. 👍 |
Thanks a lot for clarifying this @etoledom . One small thing, I was quoting myself so I was already agreeing on handling this on a separate PR :) |
WordPress/Classes/ViewRelated/Gutenberg/GutenbergViewController.swift
Outdated
Show resolved
Hide resolved
The only remaining issue is the focus-on-open problem. It's my understanding that this issue will cover it? #10594 (comment) This is ready for another round. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works and looks good! 🎉
Looking great @diegoreymendez! Two minor things:
Also curious how this looks/works in:
Thanks again, Diego! |
@iamthomasbishop - I'll be spawning an issue for the placeholder correction. There's already an issue for the border luckily. Those are next in my list of things to-do, but I'll start by merging this PR as-is, to make sure everyone can see the RN title for starters. |
IMPORTANT:
This is a work in progress. This PR is NOT ready for merging.
Description:
Implements the editor title in RN for Gutenberg, also removes the native title.
Related PRs:
gutenberg: WordPress/gutenberg#13199
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#450
Testing:
Test 1:
Make sure the post title has been udpated.
Test 2:
Make sure the post title hasn't changed.
Test 3:
Make sure the placeholder shows up and the title is empty.
Test 4:
Make sure multiline titles work fine.
Test 5:
Make sure pressing ENTER does not insert a newline. Later on we can change this to move the focus to the content but that's outside the scope of this PR.