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

Fixed matches sorting in shortcodeConverter. #4270

Merged
merged 1 commit into from Jan 4, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jan 3, 2018

It looks like the algorithm to compute and return shortcode pieces array is dependent on ascending sorting of the matches and the sorting was not ascending causing a series of bugs and unexpected behaviours.
This change should fix issue #4215 and the "Problems converting a Bootstrap page" section referred in issue #4219.

Description

This PR just changes sorting in shortcodeConverter function to be ascending as it looks like is what the algorithm expects.

How Has This Been Tested?

Paste into a simple text editor first to make sure pasting is plain text. Copying from here escapes content.
Paste the following text :

<div>
[caption id="attachment_915" align="aligncenter" width="50"]<a href="/another-page/"><img src="https://dummyimage.com/50x50/333333/09f09f.jpg&amp;text=image+1" alt="“My alt” alt 1" width="50" height="50" /></a> “My caption” caption 1[/caption]

[caption id="attachment_936" align="aligncenter" width="50"]<a href="/another-page/"><img src="https://dummyimage.com/50x50/000000/ffffff.jpg&amp;text=image+2" alt="“My alt” alt 2" width="50" height="50" /></a> “My caption” caption 2[/caption]
</div> 

Verify the output is an image block as expected.

Create a post in the classic editor with the following content provided by @ElectricFeet in issue #4219:

<div class="row">

<div class="col-xs-12 col-md-6 col-lg-4">
[caption id="attachment_915" align="aligncenter" width="400"]<a href="/another-page/"><img src="https://dummyimage.com/400x400/333333/09f09f.jpg&text=image+1" alt="“My alt” alt 1" width="400" height="400" /></a> “My caption” caption 1[/caption]
</div>

<div class="col-xs-12 col-md-6 col-lg-4">
[caption id="attachment_936" align="aligncenter" width="400"]<a href="/another-page/"><img src="https://dummyimage.com/400x400/000000/ffffff.jpg&text=image+2" alt="“My alt” alt 2" width="400" height="400" /></a> “My caption” caption 2[/caption]
</div>

<div class="col-xs-12 col-md-12 col-lg-4">
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.</p> 
<p style="text-align: right;"><a title="More" href="/another-page/">...More</a></p>
</div>

</div>

Load the post in Gutenberg use convert to blocks option and verify the result is similar to what we add in the classic editor.

Verify "simple" shortcode pasting works as before.

Screenshots (jpeg or gifs if applicable):

Before:
image
After:
image

Before:
image

After:
image

Fixed matches sorting in shortcodeConverter.
It looks like the algorithm to compute the return array is dependent on ascending sorting of the matches and the sorting was not ascending causing a series of bugs.
@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jan 4, 2018

Member

Wow, I can't believe I didn't see your PR... I just created #4274 which does essentially the same. Are the array of keys not all strings though? Should we pass through pasteInt?

Member

iseulde commented Jan 4, 2018

Wow, I can't believe I didn't see your PR... I just created #4274 which does essentially the same. Are the array of keys not all strings though? Should we pass through pasteInt?

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jan 4, 2018

Member

Are the array of keys not all strings though? Should we pass through pasteInt?

Hi @iseulde, thank you for your review. It is a good indication that this solves the problem if both of us arrived to the same fix :) Yes, the array keys are strings, but given that we are applying the "-" arithmetic operator the values get converted to integers before the operator execution, so I decided to not have parseInt in order for the code to be simpler.

Member

jorgefilipecosta commented Jan 4, 2018

Are the array of keys not all strings though? Should we pass through pasteInt?

Hi @iseulde, thank you for your review. It is a good indication that this solves the problem if both of us arrived to the same fix :) Yes, the array keys are strings, but given that we are applying the "-" arithmetic operator the values get converted to integers before the operator execution, so I decided to not have parseInt in order for the code to be simpler.

@iseulde

iseulde approved these changes Jan 4, 2018

Sounds good to me then :)

@iseulde iseulde referenced this pull request Jan 4, 2018

Closed

Fix shortcode sorting #4274

3 of 3 tasks complete

@jorgefilipecosta jorgefilipecosta merged commit 99b0387 into master Jan 4, 2018

3 checks passed

codecov/project 39.65% (+0.52%) compared to 2d559e3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/shortcode-converter-sorting branch Jan 4, 2018

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Jan 4, 2018

Contributor

Ah, late to the party. Good fix. Since it may not be obvious to the reader that Array#sort defaults to alphabetical sorting, we could add a small comment explaining that providing (a, b) => a - b just forces numerical sorting over alphabetical.

Contributor

mcsf commented Jan 4, 2018

Ah, late to the party. Good fix. Since it may not be obvious to the reader that Array#sort defaults to alphabetical sorting, we could add a small comment explaining that providing (a, b) => a - b just forces numerical sorting over alphabetical.

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