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

Reuse validation error titles from validated URL screen in block warning notice #4401

Merged
merged 8 commits into from
Mar 18, 2020
Merged

Reuse validation error titles from validated URL screen in block warning notice #4401

merged 8 commits into from
Mar 18, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Mar 17, 2020 •
edited
Loading

Summary

Fixes #3664.

Given a Custom HTML block with this content:

<script type="application/json">bads</script>
<amp-img></amp-img>
<button onclick="foo"></button>
<?bad-pi yes?>
<amp-geo layout="nodisplay"><script type="application/json">{</script></amp-geo>
<amp-geo layout="nodisplay"><script type="application/json">{}</script></amp-geo>
Before After
Screen Shot 2020-03-17 at 12 01 08 Screen Shot 2020-03-17 at 12 01 20

Also fixes the font-family of the list items to also be the system font.

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

@westonruter westonruter added this to the v1.5 milestone Mar 17, 2020
@westonruter westonruter requested a review from pierlon March 17, 2020 19:10
@googlebot googlebot added the cla: yes Signed the Google CLA label Mar 17, 2020
@westonruter westonruter mentioned this pull request Mar 17, 2020
Closed
pierlon
pierlon approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Looks good 👍

assets/src/block-validation/components/validation-error-message/index.js
if ( message ) {
return message;
return message; // @todo It doesn't appear this is ever set?
Copy link
Contributor

Choose a reason for hiding this comment

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

The REST response result doesn't include a message key, so I don't think it's set. I couldn't find any reference to it either in any other React component... maybe @swissspidy could shine a light on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems it's only be used in its test:

amp-wp/assets/src/block-validation/components/validation-error-message/test/index.js

Line 13 in 4a59e23

const errorMessage = render( <ValidationErrorMessage message="Hello World" /> );

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Mar 17, 2020
@pierlon
Copy link
Contributor

pierlon commented Mar 17, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Mar 17, 2020
pierlon and others added 5 commits March 17, 2020 19:54
@pierlon
Update test snapshot
677a9d6
@pierlon
Add whitespace to make test pass
efbad91 
(Thanks to PHPStorm for trimming it pre-commit)
@westonruter
Improve reliability of AMP_Facebook_Embed_Test::test__raw_embed_sanit…
7041680 
…izer
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/valid…
aa9593d 
…ation-error-titles-to-block-warnings

* 'develop' of github.com:ampproject/amp-wp:
  Remove title tag from AMP SVG icons
  Use a Tooltip, thanks to Pierre's suggestion
  Correct failed unit test from dependency
  Remove @wordpress/nux, as it looks like it's not used anymore
  Remove DotTip from import statements
  Remove DotTip, as it caused a console warning
  Improve AMP preview button in WP 5.4 and with Gutenberg
  Make phpstan happy
  Remove multiple instances of div#fb-root
  Test removal of <p> tag
  Eliminate unnecessary double loop for XPath query since DOMNodeList is not live
  Remove phpstan.neon.dist from build
  Use `nodeName` property
  Remove registration of Facebook embed handler
@westonruter
Normalize additional blockquotes after merge conflict
3c2ae84
@westonruter westonruter merged commit e45877b into develop Mar 18, 2020
@westonruter westonruter deleted the add/validation-error-titles-to-block-warnings branch March 18, 2020 01:04
@westonruter westonruter mentioned this pull request Mar 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierlon pierlon pierlon approved these changes

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

Improve displaying validation errors' invalid markup in Gutenberg block warning notice
3 participants
@westonruter @googlebot @pierlon