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

Fix crash occurring when we try to display block picker #997

Merged
merged 1 commit into from
May 20, 2019

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented May 17, 2019

This crash occurs when we we try to show block picker. It is not yet on the develop branch but it will come in when we update the gutenberg ref to the latest master. This looks like a side effect of: https://github.com/WordPress/gutenberg/pull/15462/files

Screen Shot 2019-05-17 at 20 03 20

To test:

  • Tap on (+) and display the block picker
  • Verify it doesn't crash
  • Verify Heading block has a new icon

Simulator Screen Shot - iPhone X - 2019-05-17 at 21 15 52

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@pinarol pinarol added [Type] Bug Something isn't working [Type] Crash labels May 17, 2019
@pinarol pinarol requested a review from Tug May 17, 2019 18:18
@pinarol pinarol self-assigned this May 17, 2019
@pinarol pinarol added this to the v1.6 milestone May 17, 2019
@Tug
Copy link
Contributor

Tug commented May 17, 2019

I think it might be a good opportunity to switch to using the BlockIcon component.
It has pretty much the same requirements. It accepts an icon props which can be both a string or an svg component.
And it's actually the component used to render the icon attribute of a block.
We should be able to override the foreground color for all blocks by passing a third argument className which will have the color we want.

@koke
Copy link
Member

koke commented May 20, 2019

I think it might be a good opportunity to switch to using the BlockIcon component.

I agree, but if this solution works, we should merge it first to unblock master, then move to BlockIcon

@pinarol
Copy link
Contributor Author

pinarol commented May 20, 2019

hey @Tug 👋 BlockIcon doesn't seem cross platform yet. I also tried with Icon but className won't work on mobile too, also style just isn't passed to the inner Dashicon or SVG.

<Icon icon={ icon.src } size={ styles.modalIcon.width } style={ { fill: color } } className={ "components-picker_icon" }/>

So it looks like these components need a bit modification to be able to achieve the results we want. Please let me know if I am missing sth.

I agree with @koke about not blocking the master, we can make other enhancements on a separate PR, wdyt?

@pinarol pinarol requested a review from etoledom May 20, 2019 09:25
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

I agree, but if this solution works, we should merge it first to unblock master, then move to BlockIcon

This solution works. Let's merge to unblock master. 👍

I agree that it will take some extra work to use the same component used in web, and we should work on that on a different PR.

@pinarol pinarol merged commit 8e97769 into develop May 20, 2019
@pinarol pinarol deleted the fix/regression-of-heading-icon-change branch May 20, 2019 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working [Type] Crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants