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

[Gutenberg Mobile] Add External Link support to React Native #23090

Merged
merged 2 commits into from
Jun 17, 2020

Conversation

enejb
Copy link
Contributor

@enejb enejb commented Jun 11, 2020

Description

Related Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#2374

Currently when you insert a Header Block you see the following error.
Screen Shot 2020-06-11 at 3 53 18 PM

The regression was introduced in a917162#diff-78f749ce9638e8f9d7190a6ac691f1b7R24
it adds the the anchor hook that uses ExternalLink component hence giving the error in the console.

This PR tries to fix this by adding a new ExternalLink Component.
Notice that this component is not visible anywhere in UI just yet.

How has this been tested?

On React Native insert a new Header Block Component.
Notice that the error doesn't appear any more.

Types of changes

  • Adds new ExternalLink Component.

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.

@enejb
Copy link
Contributor Author

enejb commented Jun 12, 2020

cc: @ geriux for a review.

@enejb
Copy link
Contributor Author

enejb commented Jun 12, 2020

I would love to add a test that where that would catch any errors when adding a new block to the editor. Any ideas how to do that? Maybe its best to wait for the monorepo to be merged in order to have that in place?

@geriux geriux self-requested a review June 15, 2020 09:22
Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding this! What do you think if we add a bit more info about this issue in this PR description? For example, saying that it was introduced by this change that added the anchor hook that uses this component hence giving the error in the console.

@geriux
Copy link
Member

geriux commented Jun 15, 2020

I would love to add a test that where that would catch any errors when adding a new block to the editor. Any ideas how to do that? Maybe its best to wait for the monorepo to be merged in order to have that in place?

Yup, as we discussed today, it'd be best to wait for the monorepo work to be merged since it'll include some tests and then we'll be able to see if we need to add new ones to catch any errors.

@geriux geriux 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 15, 2020
@geriux geriux merged commit df649c1 into WordPress:master Jun 17, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 17, 2020
@enejb enejb deleted the add/gbm-external-link branch June 17, 2020 13:02
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
Development

Successfully merging this pull request may close these issues.

2 participants