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

Blocks: Merge paragraph and cover text blocks #2452

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Aug 17, 2017

closes #2448

Notes

  • The paragraph block has Block Alignment Toolbar too but I kept only "wide" and "full" alignments. For me, the "center" block alignment is useless for a paragraph block.
  • I added a "reset" link to font-size range
  • There's no wrapper element (like the cover-text) only a p
  • Parsing a core/cover-text will result in an invalid core/paragraph block. I don't know if we should do more about this, we're still beta right.
  • The editable has a new prop wrapperClassname replacing the old className prop. It's seemed more logic to me to apply the className prop directly to the TinyMCE node.
  • Cover Text is MultiParagraph, Paragraph is not.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label Aug 17, 2017
@youknowriad youknowriad self-assigned this Aug 17, 2017
@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

Merging #2452 into master will decrease coverage by 0.11%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
- Coverage   26.98%   26.86%   -0.12%     
==========================================
  Files         159      158       -1     
  Lines        4903     4891      -12     
  Branches      817      816       -1     
==========================================
- Hits         1323     1314       -9     
+ Misses       3035     3030       -5     
- Partials      545      547       +2
Impacted Files Coverage Δ
blocks/library/preformatted/index.js 40% <ø> (ø) ⬆️
blocks/library/list/index.js 6.97% <ø> (ø) ⬆️
blocks/library/table/table-block.js 8% <ø> (ø) ⬆️
blocks/library/verse/index.js 37.5% <ø> (ø) ⬆️
blocks/library/pullquote/index.js 33.33% <ø> (ø) ⬆️
blocks/editable/tinymce.js 0% <0%> (ø) ⬆️
blocks/inspector-controls/range-control/index.js 75% <0%> (-25%) ⬇️
blocks/api/parser.js 98.11% <100%> (ø) ⬆️
blocks/editable/index.js 11.02% <100%> (ø) ⬆️
blocks/library/paragraph/index.js 29.16% <30.76%> (-17.9%) ⬇️

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 ccf991a...3d9f99c. Read the comment docs.

@jasmussen
Copy link
Contributor

Nice!

This works amazingly well. I think we should do it.

However I don't think we should include the alignments in this toolbar, I think we should either sacrifice those, or move them to the block inspector. The paragraph block should first and foremost be just that, text, and so this implicitly categorizes the layout alignments as "advanced settings".

Curious what @mtias thinks, but I'm okay with both removing the layout alignments, or moving them to the inspector (like we did with headline).

@ellatrix
Copy link
Member

I like merging these. I've been confused by cover text, for a while, as it was just a paragraph with a centre button. U just discovered now that it has more block settings buried in the inspector.

@youknowriad youknowriad force-pushed the update/merge-paragraph-cover-text branch from d81b784 to 029035c Compare August 22, 2017 09:43
@jasmussen
Copy link
Contributor

Let's move the layout alignments to the sidebar for now, and then get this merged. They are absolutely an advanced aspect of the "text" block, and so the sidebar is the appropriate place for it.

It'll also give some welcome synergy with the heading block:

screen shot 2017-08-22 at 11 48 57

Once moved there, let's merge this and fix any issues that pop up separately.

@youknowriad
Copy link
Contributor Author

@jasmussen do you think I should enable the float left/right alignments too (in the inspector) or keep only wide/full?

@jasmussen
Copy link
Contributor

do you think I should enable the float left/right alignments too (in the inspector) or keep only wide/full?

Unless it looks terrible with the left/right/center, then no reason not to. The reasons you might want to keep them out, is that they require a size that's not "editor width" in order to be meaningful, and we don't have any way of resizing the text block quite yet. I'd also be very careful in exploring too much resizing action yet, as this steps firmly into customization territory and might be best served with a more holistic approach, CC: @melchoyce

@youknowriad youknowriad merged commit 53dfb42 into master Aug 22, 2017
@youknowriad youknowriad deleted the update/merge-paragraph-cover-text branch August 22, 2017 10:27
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.

Consider merging Text and Cover Text
3 participants