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

Fix paste attributes after #1902 #2408

Merged
merged 5 commits into from Aug 15, 2017

Conversation

Projects
None yet
2 participants
@iseulde
Member

iseulde commented Aug 14, 2017

No description provided.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

@aduth Should all of these attributes have defaults set?

Member

iseulde commented Aug 14, 2017

@aduth Should all of these attributes have defaults set?

@aduth

Alternatively, I'd wondered if we just need to feed in the attributes as source instead of defining their full type, since the full type is essentially a duplicate of what should already exist in the destination block type.

diff --git a/blocks/api/paste.js b/blocks/api/paste.js
index 84285728..76e53d3c 100644
--- a/blocks/api/paste.js
+++ b/blocks/api/paste.js
@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { find, get, flowRight as compose } from 'lodash';
+import { find, get, flowRight as compose, mapValues } from 'lodash';
 
 /**
  * WordPress dependencies
@@ -96,7 +96,7 @@ export default function( nodes ) {
 
                        const attributes = getSourcedAttributes(
                                node.outerHTML,
-                               transform.attributes,
+                               mapValues( transform.attributes, ( source ) => ( { source } ) ),
                        );
 
                        return createBlock( blockType.name, attributes );

Can we add some unit tests for paste block transforms to capture these issues?

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 14, 2017

Codecov Report

Merging #2408 into master will increase coverage by 0.54%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2408      +/-   ##
==========================================
+ Coverage    25.9%   26.45%   +0.54%     
==========================================
  Files         157      157              
  Lines        4853     4858       +5     
  Branches      822      824       +2     
==========================================
+ Hits         1257     1285      +28     
+ Misses       3035     3017      -18     
+ Partials      561      556       -5
Impacted Files Coverage Δ
blocks/library/image/index.js 15% <ø> (ø) ⬆️
blocks/library/paragraph/index.js 47.05% <ø> (ø) ⬆️
blocks/library/list/index.js 6.97% <0%> (ø) ⬆️
blocks/library/heading/index.js 23.8% <0%> (ø) ⬆️
blocks/api/paste.js 97.36% <100%> (+35.82%) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 36% <0%> (+1%) ⬆️
blocks/api/paste/remove-spans.js 77.77% <0%> (+66.66%) ⬆️
blocks/api/paste/strip-attributes.js 100% <0%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77620b2...c04c554. Read the comment docs.

codecov bot commented Aug 14, 2017

Codecov Report

Merging #2408 into master will increase coverage by 0.54%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2408      +/-   ##
==========================================
+ Coverage    25.9%   26.45%   +0.54%     
==========================================
  Files         157      157              
  Lines        4853     4858       +5     
  Branches      822      824       +2     
==========================================
+ Hits         1257     1285      +28     
+ Misses       3035     3017      -18     
+ Partials      561      556       -5
Impacted Files Coverage Δ
blocks/library/image/index.js 15% <ø> (ø) ⬆️
blocks/library/paragraph/index.js 47.05% <ø> (ø) ⬆️
blocks/library/list/index.js 6.97% <0%> (ø) ⬆️
blocks/library/heading/index.js 23.8% <0%> (ø) ⬆️
blocks/api/paste.js 97.36% <100%> (+35.82%) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 36% <0%> (+1%) ⬆️
blocks/api/paste/remove-spans.js 77.77% <0%> (+66.66%) ⬆️
blocks/api/paste/strip-attributes.js 100% <0%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77620b2...c04c554. Read the comment docs.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

What about cases where they don't map? I guess we should think of it when it pops up then.

Member

iseulde commented Aug 14, 2017

What about cases where they don't map? I guess we should think of it when it pops up then.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 14, 2017

Member

Also, it should be responsibility of createBlock to fill in attribute defaults.

Member

aduth commented Aug 14, 2017

Also, it should be responsibility of createBlock to fill in attribute defaults.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

Why shouldn't we then just loop through getSourcedAttributes instead of source and see if it returns any attributes? That removes the need entirely for these raw transforms.

Member

iseulde commented Aug 14, 2017

Why shouldn't we then just loop through getSourcedAttributes instead of source and see if it returns any attributes? That removes the need entirely for these raw transforms.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

Maybe this could work if we add required, and then look if that's satisfied.

Member

iseulde commented Aug 14, 2017

Maybe this could work if we add required, and then look if that's satisfied.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

Okay, maybe not, there will always be strange cases we'll need to handle. Let's just add what you suggested.

Member

iseulde commented Aug 14, 2017

Okay, maybe not, there will always be strange cases we'll need to handle. Let's just add what you suggested.

@iseulde iseulde requested a review from aduth Aug 14, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 14, 2017

Member

Why shouldn't we then just loop through getSourcedAttributes instead of source and see if it returns any attributes? That removes the need entirely for these raw transforms.

Yeah, in the case of the raw transforms, if we know that the original content is a match, we shouldn't need to redefine attributes and instead just trigger the default parsing of getBlockAttributes.

I think I might have been a little overreaching with the renaming of matcher: to source:. In the case of raw transforms, I think matcher: makes a little more sense for what it's doing (returning boolean, maybe also match / isMatch). What do you think?

Member

aduth commented Aug 14, 2017

Why shouldn't we then just loop through getSourcedAttributes instead of source and see if it returns any attributes? That removes the need entirely for these raw transforms.

Yeah, in the case of the raw transforms, if we know that the original content is a match, we shouldn't need to redefine attributes and instead just trigger the default parsing of getBlockAttributes.

I think I might have been a little overreaching with the renaming of matcher: to source:. In the case of raw transforms, I think matcher: makes a little more sense for what it's doing (returning boolean, maybe also match / isMatch). What do you think?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 14, 2017

Member

Sounds good. Changed it.

Member

iseulde commented Aug 14, 2017

Sounds good. Changed it.

@aduth

aduth approved these changes Aug 14, 2017

Show outdated Hide outdated blocks/api/test/paste.js

@iseulde iseulde merged commit 39c6bfc into master Aug 15, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@iseulde iseulde deleted the fix/paste-matchers branch Aug 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment