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

Don't allow multiple CTA / attachment block to be pasted. #3601

Merged
merged 6 commits into from
Oct 23, 2019

Conversation

spacedmonkey
Copy link
Contributor

@spacedmonkey spacedmonkey commented Oct 22, 2019

Summary

Fixes #3575

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

@googlebot googlebot added the cla: yes Signed the Google CLA label Oct 22, 2019
@swissspidy
Copy link
Collaborator

Looks neat at first glance! 🚀

@barklund
Copy link
Contributor

Looks go to me as well (except for the lint error).

It will conflict heavily with #3600, but it's probably an easy manual merge.

* @param {string} name Name of block.
* @param {Function} createErrorNotice Bring in dispatchable createErrorNotice function.
*/
const displayPasteError = ( name, createErrorNotice ) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having to pass createErrorNotice, couldn't this just be turned into a custom React hook?

const useDisplayPasteError = ( name ) => {
	const { createErrorNotice } = useDispatch( 'core/notices' );
	// ...
}

In assets/src/stories-editor/components/context-menu/index.js you could then do:

const displayPasteError = useDisplayPasteError( pastedBlock.name );
displayPasteError();

@swissspidy
Copy link
Collaborator

Code works well so far 👍

Just resolved the merge conflicts with #3600 now

@swissspidy swissspidy added this to the v1.4 milestone Oct 23, 2019
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Works well!

@swissspidy swissspidy marked this pull request as ready for review October 23, 2019 11:08
@swissspidy swissspidy merged commit b3149bc into develop Oct 23, 2019
@swissspidy swissspidy deleted the fix/cta-attachment branch October 23, 2019 11:08
westonruter added a commit that referenced this pull request Oct 25, 2019
…-args-debugging

* 'develop' of github.com:ampproject/amp-wp: (163 commits)
  Remove entries array test after completed.
  Update dependency core-js to v3.3.4 (#3624)
  Quick UX fixes (#3611)
  Fix most pressing RTL issues (#3558)
  Add test case for broken parent relationship
  Adapt broken test
  Throw a _doing_it_wrong() when an expected parent is not found
  Change template hierarchy query arguments to public
  Reuse data-set-focus to reliably match twentytwenty keyboard focus management
  Add tabindex attribute to modals
  Guess the role of a modal based on its classes
  Disable Code Editor (#3608)
  Update dependency postcss to v7.0.20 (#3613)
  Update dependency autoprefixer to v9.7.0 (#3614)
  Update dependency browserslist to v4.7.2 (#3612)
  Add rel=preconnect link for AMP CDN
  Fix block nav padding and margin (#3610)
  Adjust Gutenberg / WordPress requirement (#3609)
  Don't allow multiple CTA / attachment block to be pasted. (#3601)
  Fix page crashing when duplicating immovable blocks (#3593)
  ...
@spacedmonkey spacedmonkey self-assigned this Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page Attachment can be copied to a Page where CTA already exists
4 participants