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

Quote: Fix new quote, transform from list block #2349

Merged
merged 4 commits into from
Aug 11, 2017
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 10, 2017

Fixes #2343

This pull request seeks to resolve issues relating to quote initialization and list transforms.

The quote block does not define a default value but assumes it to be an array, iterating by Array#map in its save implementation. While we could add a default value to the attributes definition, the approach here instead drops the Array#map since, for all I could discern, it serves no purpose (generates identical output). Instead, the value is rendered verbatim.

Further, there were some issues with list transforms, particularly for lists which do not contain any special formatting or list nesting. In these cases, list items would be plain strings, but the block's internal toBrDelimitedContent utility was not prepared to handle these cases.

Lastly, the List -> Quote transform was not transforming into the expected value type of quote and should have been wrapped in the expected array of paragraphs.

Testing instructions:

Verify that you can create a new Quote block.

Verify that you can convert from List block to Quote (and Paragraph) block with:

  • No content
  • Single list item (with and without formatted content)
  • Multiple list items (with and without formatted content)
  • Nested list items (with and without formatted content)

Should have same result, unless we were explicitly trying to strip props from root p node, in which case this is not assigned in quote block implementation, and should be the responsibility of the transform source to remove in transform value
@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Aug 10, 2017
@aduth
Copy link
Member Author

aduth commented Aug 10, 2017

Oh failing tests, you illuminate why we'd done the map, though really shouldn't be necessary facebook/react#7038.

Needed to ensure key is assigned into map result to avoid React error. Seems unnecessary since we don't need the diffing reconciliation for generating static markup, but alas.

See: facebook/react#7038
@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #2349 into master will increase coverage by 0.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2349      +/-   ##
=========================================
+ Coverage   24.69%   24.7%   +0.01%     
=========================================
  Files         152     152              
  Lines        4738    4962     +224     
  Branches      799     849      +50     
=========================================
+ Hits         1170    1226      +56     
- Misses       3014    3136     +122     
- Partials      554     600      +46
Impacted Files Coverage Δ
blocks/library/list/index.js 6.55% <0%> (-0.68%) ⬇️
blocks/library/quote/index.js 14.28% <100%> (ø) ⬆️
blocks/library/separator/index.js 42.85% <0%> (-7.15%) ⬇️
blocks/library/cover-text/index.js 29.41% <0%> (-5.59%) ⬇️
blocks/library/heading/index.js 18.91% <0%> (-4.9%) ⬇️
blocks/library/latest-posts/index.js 7.22% <0%> (-2.78%) ⬇️
blocks/library/gallery/index.js 23.8% <0%> (-1.2%) ⬇️
blocks/library/image/index.js 13.95% <0%> (+1.7%) ⬆️
blocks/library/embed/index.js 49.37% <0%> (+3.92%) ⬆️

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 31879c2...f1952ac. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor crashes to white page when toggling list to quote
1 participant