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

[RN-MOBILE] fix line height crash #23507

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Jun 26, 2020

Fixes: wordpress-mobile/WordPress-Android#12211

Description

Current Gutenberg implementation is supporting line-height without unit e.g. 'line-height':1.5
and library which we are using to convert css to react-native requires unit to be included with dimension and that scenario is leading to crash of the Aztec wrapper on Android.

However, we didn't choose to fix (patch) the library as native AztevView doesn't support lineHeight at this moment so focus on this fix was to remove lineHeight from style.

How has this been tested?

Run the Demo app and app shouldn't crash

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 Jun 26, 2020
@marecar3 marecar3 requested review from hypest and guarani June 26, 2020 10:08
@github-actions
Copy link

Size Change: 0 B

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.37 kB 0 B
build/block-directory/style-rtl.css 941 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 109 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.6 kB 0 B
build/block-library/index.js 130 kB 0 B
build/block-library/style-rtl.css 8.04 kB 0 B
build/block-library/style.css 8.04 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.9 kB 0 B
build/compose/index.js 9.62 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/index.js 9.87 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.51 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.32 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.83 kB 0 B
build/editor/style.css 3.83 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.73 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@marecar3 marecar3 requested review from SergioEstevao and removed request for guarani June 26, 2020 10:17
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!

Verified the fix and the crash if I just remove the delete style.lineHeight, nice fix!

Also, thanks for the inline comments explaining the removal, and the added demo html!

@hypest
Copy link
Contributor

hypest commented Jun 26, 2020

Rerunning jobs as the mobile E2E job suffered from the known Assertion failed: (!uv__is_closing(handle)), function uv_close, file ../deps/uv/src/unix/core.c, line 113 issue and it's not related to this PR.

@marecar3
Copy link
Contributor Author

LGTM!

Verified the fix and the crash if I just remove the delete style.lineHeight, nice fix!

Also, thanks for the inline comments explaining the removal, and the added demo html!

Thanks for the approval!

@hypest
Copy link
Contributor

hypest commented Jun 26, 2020

The CI job failed again but, since we know for sure that it's not an issue with this PR and there's already another PR to address the issue, I'll go ahead and merge.

@hypest hypest merged commit a41914a into master Jun 26, 2020
@hypest hypest deleted the rnmobile/fix-line-height-crash branch June 26, 2020 11:22
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jun 26, 2020
SergioEstevao pushed a commit that referenced this pull request Jun 26, 2020
* Fix line-height crash

* format fix

* Add block example with lineHeight attribute
SergioEstevao added a commit that referenced this pull request Jul 8, 2020
* Run pod install in react-native-editor/ios/

* [RN-MOBILE] fix line height crash (#23507)

* Fix line-height crash

* format fix

* Add block example with lineHeight attribute

* [RNMobile] Correct Buttons block width (#23616)

* Fix quote left border color in dark mode. (#23692)

* Update react-native-editor package and release notes.

* Fix translations for media options

* Load block-library lazily after translations occur to fix names of blocks in inserter.

* lint fixes

* Merge master branch to merge_rnmobile_1.31.1_to_master

Co-authored-by: Paul Von Schrottky <[email protected]>
Co-authored-by: Marko Savic <[email protected]>
Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Sérgio Estêvão <[email protected]>
Co-authored-by: Cameron Voell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
2 participants