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

Add "Text Columns" block. #2117

Merged
merged 4 commits into from
Aug 8, 2017
Merged

Add "Text Columns" block. #2117

merged 4 commits into from
Aug 8, 2017

Conversation

mtias
Copy link
Member

@mtias mtias commented Aug 1, 2017

text-columns

This adds a new block for creating columns of text (serving as the basis for eventual generic columns).

@afercia wonder how you think we should announce navigating between columns.

@mtias mtias added [Feature] Blocks Overall functionality of blocks New Block Suggestion for a new block labels Aug 1, 2017
@youknowriad
Copy link
Contributor

Do we really want this block? Will it be "deprecated" by the introduction of nested blocks in V2?

@jasmussen
Copy link
Contributor

Do we really want this block? Will it be "deprecated" by the introduction of nested blocks in V2?

It may or may not be deprecated, and your question is a good one!

We've been over how difficult it is to get columns right, and also how already today third parties can build this. We may very well want an entirely different implementation than this one. But perhaps it's good to get this in now and test it. Perhaps this can help inform how a better column implementation can work down the road. In fact we might want to merge this block in now, only to take it back out later again, same as the Cover Text block.

For that reason, I think it'd be good to test this.

@mtias
Copy link
Member Author

mtias commented Aug 1, 2017

Yes, this adds value right now with the ability to do columns of text—which is a very frequent request—while allowing us to see what works or doesn't in column editing.

@afercia
Copy link
Contributor

afercia commented Aug 1, 2017

wonder how you think we should announce navigating between columns.

@mtias ideally: First column, Second Column, Third column... 🙂
Programmatically maybe a bit easier: Column 1, Column 2, Column 3...
(maybe better also considering space constraints?)

@jasmussen
Copy link
Contributor

I realize I made a mistake in not CC'ing @melchoyce and @westonruter on this, as it clearly touches customization. Sorry about that! Will do better next time.

@mtias mtias force-pushed the add/text-columns-block branch from 4f8f091 to 41672c8 Compare August 2, 2017 18:42
/**
* WordPress dependencies
*/
import { __ } from 'i18n';
Copy link
Member

Choose a reason for hiding this comment

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

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master

@mtias mtias force-pushed the add/text-columns-block branch from 24c2804 to df61941 Compare August 8, 2017 08:00
@codecov
Copy link

codecov bot commented Aug 8, 2017

Codecov Report

Merging #2117 into master will increase coverage by 0.09%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2117      +/-   ##
==========================================
+ Coverage   23.82%   23.91%   +0.09%     
==========================================
  Files         147      149       +2     
  Lines        4563     4608      +45     
  Branches      774      782       +8     
==========================================
+ Hits         1087     1102      +15     
- Misses       2934     2960      +26     
- Partials      542      546       +4
Impacted Files Coverage Δ
blocks/library/text-columns/index.js 33.33% <33.33%> (ø)
blocks/library/image/index.js 15.38% <0%> (-0.25%) ⬇️
components/form-file-upload/index.js 71.42% <0%> (ø)
blocks/library/cover-text/index.js 36.36% <0%> (+4.78%) ⬆️

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 73858e5...5eac883. Read the comment docs.

@mtias mtias merged commit f9bcd4e into master Aug 8, 2017
@mtias mtias deleted the add/text-columns-block branch August 8, 2017 10:16
@karmatosed
Copy link
Member

Just tested this in a theme and it's awesome to see:

2017-08-08 at 11 31

@melchoyce
Copy link
Contributor

If this does make it into 1.0, do we have any plan for migrating it to a more robust set of columns in the future? This isn't necessarily how I imagined columns working once we start to work on creating layout tools.

@mtias
Copy link
Member Author

mtias commented Aug 11, 2017

What did you have in mind for the more robust set? In terms of markup, I don't think we'll have any issue as it sort of already accommodates nesting. In any case, we'll have many ways to migrate to something else.

@melchoyce
Copy link
Contributor

For example, being able to add a heading and maybe an image to each column via the same block interface. This is especially important on smaller screens, so the columns tile correctly.

@aaronware
Copy link
Contributor

aaronware commented Aug 11, 2017

This might be considered more of a "generic" column/layout controls feedback. But overall it misses things like @melchoyce mentioned, additionally I would think users should have the ability to define column widths, have the ability to reorder columns etc. This is one area we're always iterating on with Mesh

mesh-columns

Image Link if it doesn't load above

Obviously Gutenberg tackles this differently than our little plugin but once you have columns the users expectations change slightly. We set the expectation of a grid system so we lock width to a default 12/col grid that can be filtered. So the snap you see in the .gif is related to the 12 column layout

@mtias
Copy link
Member Author

mtias commented Aug 12, 2017

For example, being able to add a heading and maybe an image to each column via the same block interface.

Sure, this depends mostly on how we want to handle the UI for nesting in general. In terms of the columns, it wouldn't change anything structurally, s there should be no problem transitioning. The current markup is a section with wp-block-column divs—within those we support "Editable" alone, but in the future we can support whatever.

@melchoyce
Copy link
Contributor

@westonruter let's keep this in mind ^

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 New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants