Polish the Table block by jasmussen · Pull Request #6314 · WordPress/gutenberg

jasmussen · 2018-04-20T11:23:18Z

This branch is based off of try/better-responsive-table, but without my attempt at better responsiveness.

New toggle to decide whether table has fixed widths, off by default, as discussed in Table Block: Default Styling #2620
Surface and style the resizing tool
Makes the table more opinionated and outputs borders in the frontend. A theme can override this.

In that other branch I tried desperately to build in a responsive table feature, but it just doesn't seem possible to do in a good way that works for every WordPress theme out there. Stacking is fine, but if we can't stack as columns, you'll just end up with a mishmash of out of context rows. If we truly want a responsive table, we have to write a new one in Flexbox from the start :(

CC: @gziolo because I was working on thxis as you did some "is-selected" work, that I may have messed up in a rebase.

GIF:

gziolo

because I was working on thxis as you did some "is-selected" work, that I may have messed up in a rebase.

All good with isSelected :)

I left a few other comments to consider.

gziolo · 2018-04-23T09:43:50Z

+			{
+				'has-fixed-layout': hasFixedLayout,
+			},
+			align ? `align${ align }` : null,


align ? align${ align } : null, can be moved to the object as follows:

{ 'has-fixed-layout': hasFixedLayout, [ `align${ align }` ]: align, },

Nice 👍 👍

gziolo · 2018-04-23T09:47:49Z

 }
+
+// Style the resize handles
+.ephox-snooker-resizer-rows {


Those names look alien, is there any way to make them look as WordPress internals? :)

I'm afraid those are from deep within the core of the TinyMCE library, so we can't change them unless we change those core files.

Sounds okey, I had to double check :)

gziolo · 2018-04-23T09:48:21Z

+	PanelBody,
+	ToggleControl,
+} from '@wordpress/components';
+import classnames from 'classnames';


classnames is an external dependency.

gziolo · 2018-04-23T09:49:10Z

 				<tbody key="1">
-					<tr><td><br /></td><td><br /></td></tr>
-					<tr><td><br /></td><td><br /></td></tr>
+					<tr><td><br /></td><td><br /></td><td><br /></td></tr>


Is it for testing or do we plan to change the default table layout to to 3x2?

gziolo · 2018-04-23T09:50:10Z

 					/>
 				</BlockControls>
+				<InspectorControls>
+					<PanelBody title={ __( 'Table Settings' ) } className="blocks-table-settings">


According to the coding styles we follow this className should be blocks-table__settings, right?

jasmussen · 2018-04-23T10:44:17Z

Rebased.

jasmussen · 2018-04-30T09:44:14Z

Rebased and squashed. I think this is ready for final review.

afercia · 2018-05-01T09:15:46Z

I'd agree Gutenberg shouldn't try to make the table responsive, that's not its responsibility. More importantly, altering the CSS display property of a table also destroys its semantics, see http://adrianroselli.com/2018/02/tables-css-display-properties-and-aria.html

jasmussen · 2018-05-01T09:19:16Z

Yep, that was one of the reasons I threw in the towel on any responsiveness other than horizontal scrolling.

I think there could be a place for a "Responsive Table" block, that's built from the start to let you insert tabular data in a transforming way. But alas, it's not this block.

afercia · 2018-05-01T09:29:05Z

Nice. For that future block, take into consideration @aardrian has experimented a lot with responsive, accessible, tables ;)

jasmussen · 2018-05-01T09:56:03Z

For that future block, take into consideration @aardrian has experimented a lot with responsive, accessible, tables ;)

Definitely — though I think that's likely going to be plugin territory for the near future/v1.

gziolo · 2018-05-02T11:05:36Z

It's not always working properly with all those dimension styles applied:

I pushed a commit which fixes the failing fixture test: 77bf6ce. It looks like it also needs to be rebased. Doing it now.

jasmussen · 2018-05-16T07:05:15Z

Going to rebase when we decide on #6739 (comment)

This branch is based off of `try/better-responsive-table`, but without my attempt at better responsiveness. - New toggle to decide whether table has fixed widths, off by default, as discussed in #2620 - Surface and style the resizing tool

jasmussen · 2018-05-16T08:27:15Z

Rebased. I think this is ready for final approval. What do you think, @gziolo ?

youknowriad · 2018-05-16T09:27:38Z

+
+.ephox-snooker-resizer-bar-dragging {
+	background: $blue-medium-500;
+}


Missing an empty line here :)

youknowriad · 2018-05-16T09:28:44Z


-	save( { attributes } ) {
-		const { content, align } = attributes;
+	save( { attributes, className } ) {


There's no className prop passed to the save callback we should just remove it, it will be added automatically.

youknowriad · 2018-05-16T09:33:15Z

Looks good to me, left some small comments, Let's merge once resolved.

jasmussen · 2018-05-16T09:36:56Z

Thanks, @youknowriad for the review. I addressed it. However, I'm seeing an issue with saving and publishing. I don't know if I messed somethign up in the rebase, but the table isn't being saved, looks like :( any idea what's up?

Try to create a table in this branch, save, preview, or save and publish. It's not working
(ノ°Д°)ノ︵ ┻━┻

youknowriad

LGTM 👍

jasmussen · 2018-05-16T10:05:21Z

Yaay thank you!

aduth · 2018-05-23T13:56:59Z

 }
+
+// Style the resize handles
+.ephox-snooker-resizer-rows {


Should these styles be applied only within the context of the table block? As implemented, these will impact any table on the screen, regardless of within the block. Even if they're desirable globally, I don't think the styles should be implemented here (since a block's styles shouldn't be impacting the page as a whole).

I wish I could scope these to be table block specific only. However these elements are inserted into the root of the DOM.

I can move them to the main stylesheet if you like?

jasmussen self-assigned this Apr 20, 2018

jasmussen requested a review from gziolo April 20, 2018 11:23

jasmussen mentioned this pull request Apr 20, 2018

Table Block: Default Styling #2620

Closed

gziolo reviewed Apr 23, 2018

View reviewed changes

gziolo added this to the 2.8 milestone Apr 23, 2018

gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Apr 23, 2018

jasmussen force-pushed the update/table branch from 861c43e to 379a610 Compare April 23, 2018 10:44

jasmussen force-pushed the update/table branch 2 times, most recently from fc05085 to 6aa1ad5 Compare April 30, 2018 09:43

jasmussen requested a review from a team May 1, 2018 08:17

gziolo force-pushed the update/table branch from 77bf6ce to e74b526 Compare May 2, 2018 11:07

aduth modified the milestones: 2.8, 2.9 May 3, 2018

helen mentioned this pull request May 15, 2018

Gutenberg compatibility? 10up/mce-table-buttons#2

Open

Joen Asmussen and others added 2 commits May 16, 2018 10:23

Polish the Table block

015727f

This branch is based off of `try/better-responsive-table`, but without my attempt at better responsiveness. - New toggle to decide whether table has fixed widths, off by default, as discussed in #2620 - Surface and style the resizing tool

Core blocks: Fix fixture tests

7de00d8

jasmussen force-pushed the update/table branch from e74b526 to 7de00d8 Compare May 16, 2018 08:26

youknowriad reviewed May 16, 2018

View reviewed changes

Address feedback.

5c15b79

jasmussen modified the milestone: 2.9 May 16, 2018

Fix table block after rebase

1efc13a

youknowriad approved these changes May 16, 2018

View reviewed changes

jasmussen merged commit 24011e3 into master May 16, 2018

jasmussen deleted the update/table branch May 16, 2018 10:05

aduth reviewed May 23, 2018

View reviewed changes

helen mentioned this pull request May 23, 2018

Table block: flesh out editor features #6923

Open

jasmussen mentioned this pull request May 24, 2018

Address Andrew feedback #6930

Merged

jasmussen mentioned this pull request Aug 24, 2018

Fix table regressions. #9306

Merged

Conversation

jasmussen commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Apr 23, 2018

Uh oh!

jasmussen commented Apr 30, 2018

Uh oh!

afercia commented May 1, 2018

Uh oh!

jasmussen commented May 1, 2018

Uh oh!

afercia commented May 1, 2018

Uh oh!

jasmussen commented May 1, 2018

Uh oh!

gziolo commented May 2, 2018

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented May 16, 2018

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jasmussen commented Apr 20, 2018 •

edited

Loading