-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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). |
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. |
d81b784
to
029035c
Compare
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: Once moved there, let's merge this and fix any issues that pop up separately. |
@jasmussen 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 |
closes #2448
Notes
p
core/cover-text
will result in an invalidcore/paragraph
block. I don't know if we should do more about this, we're still beta right.wrapperClassname
replacing the oldclassName
prop. It's seemed more logic to me to apply theclassName
prop directly to the TinyMCE node.