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
[RNMobile] RichText should delete its Enter in Heading block #37279
Conversation
Size Change: +25 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
7ea8ba7
to
1e0cf70
Compare
1e0cf70
to
5b1d891
Compare
👋 @ajitbohra! GitHub added you automatically as a reviewer. I believe the PR here should only really affect the native mobile side of things but yeah, would appreciate a look/review from you if you have the time, thanks! 👋 @SiobhyB, added you as a reviewer to make sure the native mobile mobile side of this PR is OK, thanks! |
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.
I confirmed that the splitting the heading block works as expected (with no extra lines) on both Android 11 and 12 with this PR applied. 🚀
It was also interesting to learn a bit about the deleteEnter
prop, thanks for adding me as a reviewer and for the fix!
👋 @ajitbohra, sorry for re-pinging. Let me know if you're planning to take a peek at the changes in this PR, since you are marked as a code owner, or you're happy with the reviewing that happened so far. I can then go ahead and update from trunk to resolve the |
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.
Code wise LGTM 👍
Makes sense using platform check instead of forking.
Updated from trunk (and develop on the gutenberg-mobile side) and tested this still working OK on physical device and emulator, and as CIs are now green, I'm going ahead with the merge. |
Description
Fix for wordpress-mobile/gutenberg-mobile#4322
Related PRs
How has this been tested?
Test 1
Test 2
Follow the same steps as in Test 1, but on an Android 11 device/emulator. This is a test to confirm that the behavior is not broken on current Android version.
Types of changes
deleteEnter
prop to its RichText when on native mobile. I opted for a platform conditional (versus for example forking the edit.js into edit.native.js) to avoid forking the block for this kind of small tweak. Also, opted for not defaulting the RichText component todeleteEnter=true
since that would increase the impact of the change to too many other components/blocks. The fix in this PR is targeting the heading block only. If the same issue exists in other blocks, we can reconsider where to have the fix.Checklist:
*.native.js
files for terms that need renaming or removal).