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 array of paragraphs structure from the text block #2133

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Aug 1, 2017

This seems to be a remainder from the multi paragraph structure. Currently this causes a bug with the placeholders as well when initialising the editor:

this.state = {
	empty: ! props.value || ! props.value.length,
};

@ellatrix ellatrix requested a review from aduth August 1, 2017 19:46
@codecov
Copy link

codecov bot commented Aug 1, 2017

Codecov Report

Merging #2133 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   22.93%   22.99%   +0.05%     
==========================================
  Files         142      142              
  Lines        4403     4393      -10     
  Branches      749      745       -4     
==========================================
  Hits         1010     1010              
+ Misses       2859     2853       -6     
+ Partials      534      530       -4
Impacted Files Coverage Δ
blocks/library/heading/index.js 23.8% <ø> (+7.68%) ⬆️
blocks/library/pullquote/index.js 33.33% <100%> (ø) ⬆️
blocks/library/quote/index.js 14.28% <100%> (ø) ⬆️
blocks/library/paragraph/index.js 47.05% <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 96bac9a...0f3ebf3. Read the comment docs.

@youknowriad
Copy link
Contributor

Did you check the transformations to/from text?

@ellatrix
Copy link
Member Author

ellatrix commented Aug 1, 2017

Yeah. Did you encounter problems?

@youknowriad
Copy link
Contributor

@iseulde no I haven't had the time to test the branch yet. It's just that when I do this kind of changes, I often miss the switcher and have to fix it afterwards.

Will check the branch a bit later.

@ellatrix ellatrix requested a review from youknowriad August 1, 2017 20:36
@youknowriad
Copy link
Contributor

This works great for me but there's the Heading to Text transformation we could update to drop all the multi-paragraph logic, it should be way simpler now 👍

@ellatrix ellatrix force-pushed the fix/text-content-attribute branch from dc87852 to c740563 Compare August 2, 2017 15:37
@ellatrix
Copy link
Member Author

ellatrix commented Aug 2, 2017

@youknowriad Did some cleaning up.

  • Somehow the remapping of paragraphs seems weird. It should already have keys from dom-to-react... Need to have a look where they have gone.
  • Transforming a quote back to text feels weird to me. I'd expect the whole quote transformed into multiple text blocks... (I guess I also expect being able to select multiple text blocks and make a quote, see Add multi select inspector #1811.)

@mtias
Copy link
Member

mtias commented Aug 3, 2017

Is this good to go?

@ellatrix
Copy link
Member Author

ellatrix commented Aug 3, 2017

Waiting for a review :)

@youknowriad
Copy link
Contributor

I noticed a small bug, not certain that it was introduced by this PR, but when transforming a quote with one paragraph and without a citation, we should just create a text block and not a text block and an empty quote

@ellatrix
Copy link
Member Author

ellatrix commented Aug 4, 2017

@youknowriad I see this errors atm in master and works here? Still creates an empty code, but didn't change the logic there...

@ellatrix ellatrix force-pushed the fix/text-content-attribute branch from c740563 to 0f3ebf3 Compare August 4, 2017 09:27
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

@iseulde ok let's merge this, it resolves the master errors and we could remove the empty quote later

@ellatrix ellatrix merged commit 12c6401 into master Aug 4, 2017
@ellatrix ellatrix deleted the fix/text-content-attribute branch August 4, 2017 12:33
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.

3 participants