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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Activate preformat on android #18777

Merged
merged 10 commits into from
Dec 9, 2019
Merged

[RNMobile] Activate preformat on android #18777

merged 10 commits into from
Dec 9, 2019

Conversation

marecar3
Copy link
Contributor

Description

Activate pre format block on Android platform: wordpress-mobile/gutenberg-mobile#1264

GB mobile PR: wordpress-mobile/gutenberg-mobile#1615

How has this been tested?

Follow test instructions on gb-mobile PR: wordpress-mobile/gutenberg-mobile#1615

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@marecar3 marecar3 added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 27, 2019
@marecar3 marecar3 changed the base branch from master to rnmobile/release-v1.18.0 November 27, 2019 12:07
This was referenced Nov 27, 2019
Closed
Merged
@marecar3 marecar3 requested a review from hypest November 28, 2019 12:09
@marecar3 marecar3 changed the base branch from rnmobile/release-v1.18.0 to master November 28, 2019 12:53
@marecar3
Copy link
Contributor Author

Changed target branch to master as we don't want to hurry with this feature (it still has some active issues)

@hypest
Copy link
Contributor

hypest commented Nov 28, 2019

Changed target branch to master

I think this PR also needs rebasing on top of master to only include its own commits. E.g. the "Spacer" commit is not really part of the PR.

Screenshot 2019-11-28 at 15 25 24

@marecar3 marecar3 changed the title [RNMobile] - Activate preformat on android [RNMobile] Activate preformat on android Nov 28, 2019
@marecar3 marecar3 force-pushed the rnmobile/activate_pre_format_android_new branch from 2faec82 to dc7ca04 Compare November 28, 2019 22:00
@marecar3
Copy link
Contributor Author

Changed target branch to master

I think this PR also needs rebasing on top of master to only include its own commits. E.g. the "Spacer" commit is not really part of the PR.

Screenshot 2019-11-28 at 15 25 24

Thanks, @hypest I have fixed that one.

hypest
hypest reviewed Dec 5, 2019
View reviewed changes
packages/rich-text/src/component/index.native.js
} = this.props;

// aztec won't trimm spaces in a case of <pre> block, so we are excluding it
if ( tagName === 'pre' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and clear way of shortcircuiting willTrimSpaces in the Preformatted block case!

Can you also add a test in https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/component/test/index.native.js to cover that case? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, thanks!

hypest
hypest reviewed Dec 5, 2019
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

This works nicely! Let's add the unit test for the new willTrimSpaces codepath and fix the minor typo and I can review again!

@marecar3
Copy link
Contributor Author

marecar3 commented Dec 5, 2019

This works nicely! Let's add the unit test for the new willTrimSpaces codepath and fix the minor typo and I can review again!

Hey @hypest, I have added unit test and also I have fixed minor typo.
You can do another iteration of review :)

@hypest
Copy link
Contributor

hypest commented Dec 6, 2019

👋 @marecar3 , seems that the PR needs updating from master and there are a couple of lint issues to fix too.

@marecar3
Merge branch 'master' into rnmobile/activate_pre_format_android_new
64e372e 
# Conflicts:
#	packages/block-library/src/index.native.js
@marecar3
Copy link
Contributor Author

marecar3 commented Dec 6, 2019

👋 @marecar3 , seems that the PR needs updating from master and there are a couple of lint issues to fix too.

👋 @hypest , I have update this PR with the latest master and I am not sure about lint issues, as commit before this one was with ✅ https://github.com/WordPress/gutenberg/runs/336677088
but maybe I am missing something.

@hypest
Copy link
Contributor

hypest commented Dec 6, 2019

Sorry, I think I didn't notice the commit that fixed them before! Sorry for the confusion 😞

hypest
hypest approved these changes Dec 6, 2019
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM!

@marecar3 marecar3 mentioned this pull request Dec 7, 2019
Merged
1 task
SergioEstevao
SergioEstevao approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

:shipit:

@marecar3 marecar3 merged commit 585c4e9 into master Dec 9, 2019
@marecar3 marecar3 deleted the rnmobile/activate_pre_format_android_new branch December 9, 2019 15:25
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergioEstevao SergioEstevao SergioEstevao approved these changes

@hypest hypest hypest approved these changes

@maxme maxme Awaiting requested review from maxme

@mchowning mchowning Awaiting requested review from mchowning

@cameronvoell cameronvoell Awaiting requested review from cameronvoell

Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 7.1
Development

Successfully merging this pull request may close these issues.
4 participants
@marecar3 @hypest @SergioEstevao @youknowriad