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] Fix crash once adding Group #19457

Merged
merged 1 commit into from
Jan 10, 2020
Merged

[RNMobile] Fix crash once adding Group #19457

merged 1 commit into from
Jan 10, 2020

Conversation

lukewalczak
Copy link
Member

@lukewalczak lukewalczak commented Jan 7, 2020 •
edited
Loading

Description

Fix a crash once adding a Group block.

Ref to gutenberg mobile: wordpress-mobile/gutenberg-mobile#1755

How has this been tested?

  1. Add a Group block
  2. Check if block is added correctly

Screenshots

before after
Screenshot 2020-01-07 at 12 04 35 Screenshot 2020-01-07 at 12 02 24

Types of changes

Check if icon exists before passing into cloneElement.

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. .

@lukewalczak lukewalczak mentioned this pull request Jan 7, 2020
Merged
2 tasks
@lukewalczak lukewalczak self-assigned this Jan 7, 2020
@lukewalczak lukewalczak added the [Type] Bug An existing feature does not function as intended label Jan 7, 2020
@hypest hypest requested a review from Tug January 7, 2020 11:15
@pinarol
Copy link
Contributor

pinarol commented Jan 9, 2020

👋 Are we going to merge this one or was this forgotten?

@pinarol
Copy link
Contributor

pinarol commented Jan 9, 2020 •
edited
Loading

BTW, I don't have a strong opinion about whether we should merge this to the release branch or not. So far, the crash was only observable with Group block which is not live yet. However, the fix is on Button component. So, if we are confident with the fix we can merge it to prevent any potential problems.

@lukewalczak
Copy link
Member Author

@pinarol The PR is not approved yet, that's why it's not merged.
I'm not a decisive person, but that fix is preventing from a serious crash. I have the feeling that in the worst scenario probably it's better to not display Icon than have a crash.

pinarol
pinarol approved these changes Jan 10, 2020
View reviewed changes
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.

LGTM! 🎉 Tested with WPiOS by adding a Group block

@lukewalczak lukewalczak merged commit ec2d006 into rnmobile/release-v1.20.0 Jan 10, 2020
@lukewalczak lukewalczak deleted the fix-group-crash branch January 10, 2020 09:56
@Tug Tug mentioned this pull request Jan 10, 2020
Merged
6 tasks
Tug added a commit that referenced this pull request Jan 10, 2020
@Tug @lukewalczak
@dratwas @koke
[RNMobile] Merge mobile release v1.20.0 back into master (#19562)
041394d 
* styling fixes after navigation feature merge (#19455)

* Styling fixes to navigation feature

* Add netural styles for toolbar

* Fix condition for not registered component

* Display 'Unsupported' in breadcrumbs for missing components

* Refactor after CR

* Remove leftovers

* [FIX] rich text focus loop (#19240)

* check if onBlur event contains text that is different than value

* add check if there is a text in native event

* Prevent re-selection of RichText when native selection changes as a result of resigning focus

* Fix typo

* Check if isSelected only in onSelectionChangeFromAztec

Co-authored-by: Jorge Bernal <[email protected]>

* [RNMobile] Correct displaying photo during upload (#19502)

* [RNMobile] Fix crash once adding Group (#19457)

* Add extra branch for travis to run tests onto

Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>
This was referenced Jan 10, 2020
Closed
Merged
@pinarol pinarol mentioned this pull request Jan 14, 2020
Merged
2 tasks
Tug added a commit that referenced this pull request Jan 14, 2020
@Tug @lukewalczak
@dratwas @koke
[RNMobile] Merge mobile release v1.20.0 back into master (step 2) (#1…
adaae10 
…9569)

* [RNMobile] Merge mobile release v1.20.0 back into master (#19562)

* styling fixes after navigation feature merge (#19455)

* Styling fixes to navigation feature

* Add netural styles for toolbar

* Fix condition for not registered component

* Display 'Unsupported' in breadcrumbs for missing components

* Refactor after CR

* Remove leftovers

* [FIX] rich text focus loop (#19240)

* check if onBlur event contains text that is different than value

* add check if there is a text in native event

* Prevent re-selection of RichText when native selection changes as a result of resigning focus

* Fix typo

* Check if isSelected only in onSelectionChangeFromAztec

Co-authored-by: Jorge Bernal <[email protected]>

* [RNMobile] Correct displaying photo during upload (#19502)

* [RNMobile] Fix crash once adding Group (#19457)

* Add extra branch for travis to run tests onto

Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>

* Revert travis changes

Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
hypest added a commit that referenced this pull request Feb 11, 2020
@hypest @Tug
@lukewalczak @dratwas @koke @etoledom @mkevins @chipsnyder
[RNMobile] Bring release v1.22.0 to master (#20155)
7dda20a 
* [RNMobile] Merge mobile release v1.20.0 back into master (#19562)

* styling fixes after navigation feature merge (#19455)

* Styling fixes to navigation feature

* Add netural styles for toolbar

* Fix condition for not registered component

* Display 'Unsupported' in breadcrumbs for missing components

* Refactor after CR

* Remove leftovers

* [FIX] rich text focus loop (#19240)

* check if onBlur event contains text that is different than value

* add check if there is a text in native event

* Prevent re-selection of RichText when native selection changes as a result of resigning focus

* Fix typo

* Check if isSelected only in onSelectionChangeFromAztec

Co-authored-by: Jorge Bernal <[email protected]>

* [RNMobile] Correct displaying photo during upload (#19502)

* [RNMobile] Fix crash once adding Group (#19457)

* Add extra branch for travis to run tests onto

Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>

* Adding empty function to RichText children call. (#19818)

This fixes a crash originated on this PR:
#19536
`

* Disable gallery image size options on mobile (#19828)

* [Mobile] Fix blank image size labels on mobile (#19800) (#20045)

* Fix blank image size labels on mobile

* Use name instead of label in default imageSizes

* [RNMobile] Enable Dismiss on PlainText in Android (#20095)

* Add flag for determining if running on Android

* Enable Dismiss button on PlainText. Enable show keyboard in Android on PlainText mount

* Enable Dismiss button on PlainText. Enable show keyboard in Android on PlainText mount

Co-authored-by: Tugdual de Kerviler <[email protected]>
Co-authored-by: Luke Walczak <[email protected]>
Co-authored-by: Drapich Piotr <[email protected]>
Co-authored-by: Jorge Bernal <[email protected]>
Co-authored-by: etoledom <[email protected]>
Co-authored-by: Matthew Kevins <[email protected]>
Co-authored-by: Chip <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinarol pinarol pinarol approved these changes

@Tug Tug Awaiting requested review from Tug

Assignees

@lukewalczak lukewalczak

Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 7.3
Development

Successfully merging this pull request may close these issues.
3 participants
@lukewalczak @pinarol @ellatrix