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

Fixed quote<->list transformations #3525

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Nov 16, 2017

Description

This PR aims to fix a bug that I just discovered. Transforming from a quote into a list throws an error, and transforming a list into a quote creates a quote with empty content.
This PR solves both problems and simplifies the logic in a similar way of what was done in paragraph<->list transformations.
Now, transforming from quote to list, creates a list of the quote paragraphs and the last item being the citation. Transforming a list into a quote creates a paragraph for each list item excluding the last one that is used as a citation. This makes the transformations (quote->list->quote, list->quote->list)
totally idempotent.

How Has This Been Tested?

Transform a quote (empty and non-empty) into a list. Verify the first two values of the list are quote, citation.
Transform a list (empty and non-empty) into a quote. Verify the first two values of the list are now quote and citation of the quote block. If the list contains more than two values just the first two should be used.

Screenshots (jpeg or gifs if applicable):

Before:

List to quote
nov-16-2017 19-48-38

Quote to list
nov-16-2017 19-50-43

After:

nov-17-2017 11-54-52

@jorgefilipecosta jorgefilipecosta self-assigned this Nov 16, 2017
@jorgefilipecosta jorgefilipecosta force-pushed the fix/fixed-quote-list-transformations branch 3 times, most recently from e2d62fc to 14392b2 Compare November 16, 2017 21:31
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code looks good and works perfectly fine for two items. However, whenever you have more than 2 items on the list or more than two paragraphs in the quote, they are removed when transforming. See screencast:

list-quote

@jorgefilipecosta jorgefilipecosta force-pushed the fix/fixed-quote-list-transformations branch from 14392b2 to 8d8f484 Compare November 17, 2017 11:51
@codecov
Copy link

codecov bot commented Nov 17, 2017

Codecov Report

Merging #3525 into master will increase coverage by 0.95%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3525      +/-   ##
=========================================
+ Coverage   34.85%   35.8%   +0.95%     
=========================================
  Files         261     262       +1     
  Lines        6717    6862     +145     
  Branches     1225    1274      +49     
=========================================
+ Hits         2341    2457     +116     
- Misses       3691    3716      +25     
- Partials      685     689       +4
Impacted Files Coverage Δ
blocks/library/list/index.js 6.89% <0%> (ø) ⬆️
blocks/library/shortcode/index.js 33.33% <0%> (-6.67%) ⬇️
blocks/library/html/index.js 14.28% <0%> (-2.39%) ⬇️
blocks/library/more/index.js 20% <0%> (-2.23%) ⬇️
editor/store-persist.js 100% <0%> (ø) ⬆️
blocks/hooks/index.js 100% <0%> (ø) ⬆️
...or/components/block-inspector/advanced-controls.js 0% <0%> (ø)
blocks/api/parser.js 98.48% <0%> (+0.48%) ⬆️
blocks/api/serializer.js 98.85% <0%> (+0.66%) ⬆️
blocks/hooks/anchor.js 80.76% <0%> (+0.76%) ⬆️
... and 5 more

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 5c93d78...3bf5a17. Read the comment docs.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/fixed-quote-list-transformations branch from 8d8f484 to f6f7a49 Compare November 17, 2017 12:03
@jorgefilipecosta jorgefilipecosta force-pushed the fix/fixed-quote-list-transformations branch from f6f7a49 to 3bf5a17 Compare November 17, 2017 12:04
value: [ <p key="list">{ toBrDelimitedContent( values ) }</p> ],
value: ( values.length === 1 ? values : initial( values ) )
.map( ( value ) => ( { children: <p> { get( value, 'props.children' ) } </p> } ) ),
citation: ( values.length === 1 ? undefined : get( last( values ), 'props.children' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@gziolo
Copy link
Member

gziolo commented Nov 17, 2017

It works perfectly fine now. 🙇

@jorgefilipecosta jorgefilipecosta merged commit 804ab1e into master Nov 17, 2017
@jorgefilipecosta jorgefilipecosta deleted the fix/fixed-quote-list-transformations branch November 17, 2017 12:38
@jorgefilipecosta
Copy link
Member Author

Thank you for your review @gziolo. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants