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

In editor, make "Review issues" link open in a new tab #5319

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Sep 2, 2020

Summary

This relates to #5304, but I'm not sure it fixes it.

In the editor, various incarnations of this notice display when there are AMP validation issues at the post's URL:

Screen Shot 2020-09-02 at 12 45 47 PM

Previously, this would open in the same tab. Now this will open in a new tab.

Unfortunately, what we are able to do with that link is very limited, so I didn't add the usual icon to visually indicate it would open in a new tab. There is an __unstableHTML parameter we can add to the notice options to allow HTML strings to be passed into the notice, but the inline documentation for notices states very emphatically not to use it.

For similar reasons, we have to use an onClick handler instead of href with a target attribute to open the link in a new tab.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@google-cla google-cla bot added the cla: yes Signed the Google CLA label Sep 2, 2020
@westonruter westonruter added this to the v2.0.1 milestone Sep 2, 2020
@johnwatkins0
Copy link
Contributor Author

I have to look into these test failures, which I don't think are directly related to this change:

1) Test_AMP_Core_Block_Handler::test_register_and_unregister_embed
Failed asserting that '<div class="wp-block-categories-dropdown wp-block-categories"><select name="cat" id="wp-block-categories-2" class="postform">\n
	<option value="-1">Select Category</option>\n
	<option class="level-0" value="1">Uncategorized  (3)</option>\n
</select>\n
</div>' contains "onchange".
/tmp/wordpress/src/wp-content/plugins/amp/tests/php/src/Helpers/AssertContainsCompatibility.php:31
/tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-class-amp-core-block-handler.php:95
2) AmpProject\AmpWP\Tests\ReaderThemeLoaderTest::test_disable_widgets
Failed asserting that an array contains 'widgets'.
/tmp/wordpress/src/wp-content/plugins/amp/tests/php/src/ReaderThemeLoaderTest.php:205

@westonruter
Copy link
Member

It may be an issue introduced in the latest Gutenberg. I'll check it out.

@westonruter
Copy link
Member

The test failure doesn't happen in Gutenberg v8.8.0. It does happen in v8.9.0. Looking into what changed.

@westonruter
Copy link
Member

Using git-bisect identified the failure to be introduced by WordPress/gutenberg@7fbdda2.

@westonruter
Copy link
Member

This looks like a bug with Gutenberg. It's exporting the div block container, but not the script that comes after. Going to open a Gutenberg PR to fix.

@johnwatkins0 johnwatkins0 marked this pull request as ready for review September 2, 2020 20:38
@johnwatkins0 johnwatkins0 self-assigned this Sep 2, 2020
@johnwatkins0 johnwatkins0 added the WS:UX Work stream for UX/Front-end label Sep 2, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2020

Plugin builds for 54c0bde are ready 🛎️!

@westonruter
Copy link
Member

Going to open a Gutenberg PR to fix.

See WordPress/gutenberg#25026.

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter westonruter merged commit 1c7f590 into develop Sep 3, 2020
@westonruter westonruter deleted the feature/5304-open-link-in-new-window branch September 3, 2020 02:24
westonruter added a commit that referenced this pull request Sep 3, 2020
Co-authored-by: John Watkins <johnwatkins0@gmail.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants