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

Polish the Table block #6314

Merged
merged 4 commits into from
May 16, 2018
Merged

Polish the Table block #6314

merged 4 commits into from
May 16, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Apr 20, 2018

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:

fixed width

@jasmussen jasmussen self-assigned this Apr 20, 2018
@jasmussen jasmussen requested a review from gziolo April 20, 2018 11:23
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

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.

{
'has-fixed-layout': hasFixedLayout,
},
align ? `align${ align }` : null,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍 👍

}
}

// Style the resize handles
.ephox-snooker-resizer-rows {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds okey, I had to double check :)

PanelBody,
ToggleControl,
} from '@wordpress/components';
import classnames from 'classnames';
Copy link
Member

Choose a reason for hiding this comment

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

classnames is an external dependency.

@@ -32,14 +38,18 @@ export const settings = {
selector: 'table',
default: [
<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>
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -69,22 +90,40 @@ export const settings = {
onChange={ updateAlignment }
/>
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Table Settings' ) } className="blocks-table-settings">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.

@gziolo gziolo added this to the 2.8 milestone Apr 23, 2018
@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Apr 23, 2018
@jasmussen
Copy link
Contributor Author

Rebased.

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

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

@jasmussen jasmussen requested a review from a team May 1, 2018 08:17
@afercia
Copy link
Contributor

afercia commented May 1, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

afercia commented May 1, 2018

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

@jasmussen
Copy link
Contributor Author

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
Copy link
Member

gziolo commented May 2, 2018

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

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

@jasmussen
Copy link
Contributor Author

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

Joen Asmussen and others added 2 commits May 16, 2018 10:23
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
Copy link
Contributor Author

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


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

Choose a reason for hiding this comment

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

Missing an empty line here :)

isSelected={ isSelected }
/>
</Fragment>
);
},

save( { attributes } ) {
const { content, align } = attributes;
save( { attributes, className } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@youknowriad
Copy link
Contributor

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

@jasmussen
Copy link
Contributor Author

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
(ノ°Д°)ノ︵ ┻━┻

@jasmussen jasmussen modified the milestone: 2.9 May 16, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jasmussen
Copy link
Contributor Author

Yaay thank you!

@jasmussen jasmussen merged commit 24011e3 into master May 16, 2018
@jasmussen jasmussen deleted the update/table branch May 16, 2018 10:05
}
}

// Style the resize handles
.ephox-snooker-resizer-rows {
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants