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

Remove wrapper from image raw transform; Added test cases; #4740

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jan 29, 2018

With the latest code changes the wrapper in the image raw transform is no longer required. Added integration tests cases to make sure we don't regress on image raw handling and existing bugs don't come back. Previous bugs affecting the functionality: #4694, #4449

How Has This Been Tested?

Past multiple images in a post, verify the correct URLs are used.
Create a block with multiple images in the classic editor, use the convert to blocks option work as expected.

@jasmussen
Copy link
Contributor

Nice work, this will be a good fix to have in. It works for me! 👍 👍

@@ -18,6 +18,8 @@ const types = [
'google-docs',
'ms-word',
'ms-word-online',
'one-image',
Copy link
Member

Choose a reason for hiding this comment

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

Is there any point in adding a test for one image?

Copy link
Member Author

Choose a reason for hiding this comment

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

Normally I like tests for single and multiple values. So if the two images test fails, we can check if the single one passed if yes, we know it is problem affecting just the multiple case logic. If the single case also fails we know that it is a problem affecting the transform in general.

@ellatrix
Copy link
Member

Pasting aligned content from the old editor no longer works since it is wrapped in a paragraph. #4660 would fix that though.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 2, 2018

Hi @iseulde,

Pasting aligned content from the old editor no longer works since it is wrapped in a paragraph. #4660 would fix that though.

I'm not being able to replicate that problem.

I tried with the following content

<p style="text-align: right;">test1</p>
<p style="text-align: right;"><img class="wp-image-5114 size-medium" src="http://localhost/wp-content/uploads/2018/01/Dec-08-2017-15-12-24-17-300x137.gif" alt="" width="300" height="137" /></p>
<p style="text-align: right;">test2</p>
<p style="text-align: right;"><img class="wp-image-5109 size-medium" src="http://localhost/wp-content/uploads/2018/01/Dec-05-2017-17-52-09-9-300x248.gif" alt="" width="300" height="248" /></p>
<p style="text-align: right;">test3</p>

And the images were pasted as expected, the alignment was not right but I think it is not related it this changes. Do you have a sample where this fix is failing so I can look further :)?

@jorgefilipecosta jorgefilipecosta force-pushed the fix/multiple-images-transform branch from ed44733 to 773f27c Compare February 2, 2018 17:04
@ellatrix
Copy link
Member

ellatrix commented Feb 5, 2018

@jorgefilipecosta I meant aligned images, not aligned text. :)

@jorgefilipecosta
Copy link
Member Author

@jorgefilipecosta I meant aligned images, not aligned text. :)

Hi @iseulde, sorry I did explain the code block. What I wanted to mean is that it looks like there are two ways classic editor may align images.
One where the images are wrapped in a paragraph:

<p style="text-align: right;"><img class="wp-image-5114 size-medium" src="http://localhost/wp-content/uploads/2018/01/Dec-08-2017-15-12-24-17-300x137.gif" alt="" width="300" height="137" /></p>
<p style="text-align: right;"><img class="wp-image-5109 size-medium" src="http://localhost/wp-content/uploads/2018/01/Dec-05-2017-17-52-09-9-300x248.gif" alt="" width="300" height="248" /></p>

And using a class:

<img class="alignnone size-medium wp-image-2725 alignright" src="http://localhost/wp-content/uploads/2017/11/image-4-300x231.png" alt="" width="300" height="231" />
<img class="alignnone size-medium wp-image-17 alignright" style="font-size: 1rem;" src="http://localhost/wp-content/uploads/2017/10/eagle-2657888_640-3-300x200.jpg" alt="" width="300" height="200" />

I tried to handle both formats and both of them worked. So I'm having troubles replicating the problem when pasting aligned content from the old editor.

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2018

Do you copy from the visual editor? It has paragraphs added. Here's debug info:

Received HTML:

 <meta charset='utf-8'><p><img class="alignnone size-medium wp-image-897" src="http://localhost:8000/wp/wp-content/uploads/2018/02/5120-300x169.jpg" alt="" width="300" height="169" /></p><p><img class="alignright size-medium wp-image-219" src="http://localhost:8000/wp/wp-content/uploads/2018/01/image-10-300x180.png" alt="" width="300" height="180" /></p>
index.js:335 Received plain text:

 

index.js:135 Processed HTML piece:

 <p><img class="alignnone" src="http://localhost:8000/wp/wp-content/uploads/2018/02/5120-300x169.jpg" alt=""></p><p><img class="alignright" src="http://localhost:8000/wp/wp-content/uploads/2018/01/image-10-300x180.png" alt=""></p>

The classes are there, but the new code doesn't catch it.

If we merge #4660 first though, there is no need to account for it anymore though. There won't be any images wrapped in a paragraph tag then.

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2018

It seems #4660 and this PR overlap a bit too. #4660 will fix multiple images too, but by never having them in the same wrapper. :) This PR would correctly clean up the transformation because the wrapper logic would no longer be needed. :)

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2018

@jorgefilipecosta I think this is good after a rebase with #4660. It will fix the issue above. While pasting multiple images now works in master, this change is still a good one because there is no longer the case where an image can be wrapped in a paragraph.

The url of the first image was used in all the other images, html copy past operations, and when using covert to blocks option.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/multiple-images-transform branch from 773f27c to a40e4eb Compare February 8, 2018 14:48
@jorgefilipecosta jorgefilipecosta changed the title Fixed multiple image handling bug. Remove wrapper from image raw transform; Added test cases; Feb 8, 2018
@jorgefilipecosta jorgefilipecosta merged commit e2c824a into master Feb 8, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/multiple-images-transform branch February 8, 2018 14:59
@jorgefilipecosta
Copy link
Member Author

Thank you @iseulde it was merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Paste [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants