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

Rename core/text to core/paragraph #2135

Merged
merged 3 commits into from
Aug 3, 2017
Merged

Conversation

nylen
Copy link
Member

@nylen nylen commented Aug 1, 2017

To make it clearer that the Text block is actually a single paragraph rather than a section of continuous text, we should rename it to Paragraph.

Split out from #1959, which also introduces a multi-paragraph text block (this PR does not).

To ease potential future rebases, most of the refactoring here was done in a semi-automated fashion using the following commands and the codemod tool:

git mv blocks/library/text/ blocks/library/paragraph/
cd blocks/test/fixtures/
for i in core__text__*; do git mv "$i" "core__paragraph__${i#core__text__}"; done
cd ../../..
rm -r */build/ coverage/
codemod --extensions js,html,json,php 'core/text(?!-)' 'core/paragraph'
codemod --extensions js "'\\./text'" "'./paragraph'"
# `git add` the changed files

@nylen nylen requested review from mtias and aduth August 1, 2017 20:16
@codecov
Copy link

codecov bot commented Aug 1, 2017

Codecov Report

Merging #2135 into master will increase coverage by 0.05%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2135      +/-   ##
==========================================
+ Coverage   22.99%   23.05%   +0.05%     
==========================================
  Files         141      141              
  Lines        4370     4372       +2     
  Branches      738      739       +1     
==========================================
+ Hits         1005     1008       +3     
+ Misses       2841     2840       -1     
  Partials      524      524
Impacted Files Coverage Δ
blocks/library/quote/index.js 14.28% <0%> (ø) ⬆️
blocks/library/list/index.js 8.21% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/heading/index.js 16.12% <0%> (ø) ⬆️
blocks/library/preformatted/index.js 44.44% <0%> (ø) ⬆️
blocks/library/verse/index.js 37.5% <0%> (ø) ⬆️
blocks/api/parser.js 97.14% <100%> (+0.17%) ⬆️
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 1b322e9...823fe5a. Read the comment docs.

@ellatrix
Copy link
Member

ellatrix commented Aug 1, 2017

Sounds good to me!

@nylen nylen added this to the Beta 0.7.0 milestone Aug 1, 2017
@jasmussen
Copy link
Contributor

jasmussen commented Aug 2, 2017

I can understand the sentiment here — call it what it is. I can also understand why you'd want to make this change sooner rather than later, as it invalidates all existing blocks.

However I'm not 100% a fan of making this change now. I feel like there are still many very rough edges around text formatting, specifically around making lists and quotes, that need polish. We have a path forward on that, but it's very conceivable we'll be revisiting some of the behavior around the text block. Even now, you can make a single linebreak and you're still inside the same text block:

Loem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.
Loem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.

This is semantically typographically two paragraphs, but it'd still be part of the same text block. Edit: realized that the preceding statement was wrong on the web, that above is a br split. But it's still true for desktop based editors, where every linebreak makes a paragraph, even if they can hug each other through zero margins.

What I'm trying to say is that I feel like there's not a clear benefit to making this change yet — there might be once the linebreak and list-making flow plays out. I would also suggest that though the intention is right, make the breaking changes sooner, it is still a beta and we need to try and keep a cool head and stay nimble, there are likely to be breaking changes in the future also.

(And even breaking changes shouldn't be too much worse than the text becoming part of the Classic Text block)

@nylen
Copy link
Member Author

nylen commented Aug 2, 2017

I can understand the sentiment here — call it what it is.

The idea is from a comment on WPTavern - I think it's a really good one.

I feel like there are still many very rough edges around text formatting, specifically around making lists and quotes, that need polish.

I think both of these things should be improved independently.

it's very conceivable we'll be revisiting some of the behavior around the text block.

If and when we do this, it should happen in a separate block (core/text vs core/paragraph). This PR opens the way forward for various options there, such as allowing users a choice of behavior based on which blocks they choose.

In the meantime, plugins should be able to explore those options themselves. That's a bit difficult at the moment: there is this Text block which is not really a continuous section of text, so what do I call my plugin's text block? Actually Text?

@nylen
Copy link
Member Author

nylen commented Aug 3, 2017

In the absence of further feedback I plan to merge this later today. If and when text behavior changes to make this less necessary or less valid, we can change it back.

As usage grows, we should start thinking more about breaking changes. Fortunately this case is pretty easy to handle because the blocks have the same structure: just convert the old blocks to the new name whenever we load a post. This is done in this PR, and future PRs can use similar approaches.

@mtias
Copy link
Member

mtias commented Aug 3, 2017

Could set up a way to no invalidate core/text and set it up as an alias for now? Specially if we were to change this behaviour, we could be invalidating a lot of posts twice, for no super solid reason yet.

@nylen
Copy link
Member Author

nylen commented Aug 3, 2017

The current behavior should just be an invisible change, but it's probably possible to not modify existing content. What would be the benefit of that approach?

@youknowriad
Copy link
Contributor

If we want to surface this, maybe we should just rename the title of the block and leave its name as is. This is still a moving part let's not take a final decision here.

We already have the Classic Text with a core/freeform name

@nylen nylen mentioned this pull request Aug 3, 2017
@nylen
Copy link
Member Author

nylen commented Aug 3, 2017

A paragraph is a paragraph, so we should call it a paragraph. Having the external name different from the internal name doesn't sound like a great idea to me.

I think there is also some benefit to converting "text blocks as they exist today" to core/paragraph. Then we can lock this behavior in place and explore any new behaviors with a different block type.

I don't anticipate any adverse effects from this change either way. It's not intended to be a final decision, and it'll be easy to change it later.

@nylen
Copy link
Member Author

nylen commented Aug 3, 2017

We already have the Classic Text with a core/freeform name

This is a separate issue... but I do think these two names should match as well.

@nylen nylen force-pushed the update/rename-text-to-paragraph branch from de64fa1 to 823fe5a Compare August 3, 2017 19:17
@nylen nylen merged commit 5cd0d9c into master Aug 3, 2017
@nylen nylen deleted the update/rename-text-to-paragraph branch August 3, 2017 22:57
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.

5 participants