Skip to content

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

Merged
westonruter merged 4 commits intodevelopfrom
feature/5304-open-link-in-new-window
Sep 3, 2020
Merged

In editor, make "Review issues" link open in a new tab#5319
westonruter merged 4 commits intodevelopfrom
feature/5304-open-link-in-new-window

Conversation

@johnwatkins0
Copy link
Copy Markdown
Contributor

@johnwatkins0 johnwatkins0 commented Sep 2, 2020 •
edited
Loading

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
Copy Markdown
Contributor Author

johnwatkins0 commented Sep 2, 2020

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
Copy Markdown
Member

westonruter commented Sep 2, 2020

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

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Sep 2, 2020

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
Copy Markdown
Member

westonruter commented Sep 2, 2020

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

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Sep 2, 2020

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
Copy Markdown
Contributor

github-actions Bot commented Sep 2, 2020 •
edited
Loading

Plugin builds for 54c0bde are ready 🛎️!

@westonruter westonruter mentioned this pull request Sep 2, 2020
Merged
6 tasks
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Sep 2, 2020

Going to open a Gutenberg PR to fix.

See WordPress/gutenberg#25026.

@westonruter westonruter mentioned this pull request Sep 2, 2020
Merged
6 tasks
@westonruter westonruter mentioned this pull request Sep 3, 2020
Closed
pierlon
pierlon suggested changes Sep 3, 2020
View reviewed changes
Comment thread assets/src/block-validation/helpers/index.js Outdated
@westonruter @pierlon
Remove redundant class names
54c0bde 
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
@westonruter @johnwatkins0 @pierlon
In editor, make "Review issues" link open in a new tab (#5319)
8b1e3a0 
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
@johnwatkins0 johnwatkins0 mentioned this pull request Sep 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

+1 more reviewer

@pierlon pierlon pierlon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@johnwatkins0 johnwatkins0

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

Milestone

v2.0.1

Development

Successfully merging this pull request may close these issues.

3 participants

@johnwatkins0 @westonruter @pierlon