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.

Sign up for GitHub

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

Jump to bottom

Adds fixed table layout to table block #18870

Closed
wants to merge 3 commits into from
Closed

Adds fixed table layout to table block #18870

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69920ec
Adds fixed table layout to table block
Dec 2, 2019
4086e47
Removing the option
Dec 4, 2019
60b5f27
Changes from review
Dec 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions packages/block-library/src/table/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
"name": "core/table",
"category": "formatting",
"attributes": {
"hasFixedLayout": {
"type": "boolean",
"default": false
},
"backgroundColor": {
"type": "string"
},
Expand Down
6 changes: 0 additions & 6 deletions packages/block-library/src/table/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ const supports = {
const deprecated = [
{
attributes: {
hasFixedLayout: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The fixed layout option shouldn't be removed from the block deprecation, it's important it remains so that old versions of the block don't receive a validation error.

type: 'boolean',
default: false,
},
backgroundColor: {
type: 'string',
},
Expand Down Expand Up @@ -116,7 +112,6 @@ const deprecated = [
supports,
save( { attributes } ) {
const {
hasFixedLayout,
head,
body,
foot,
Expand All @@ -131,7 +126,6 @@ const deprecated = [
const backgroundClass = getColorClassName( 'background-color', backgroundColor );

const classes = classnames( backgroundClass, {
'has-fixed-layout': hasFixedLayout,
'has-background': !! backgroundClass,
} );

Expand Down
18 changes: 0 additions & 18 deletions packages/block-library/src/table/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ export class TableEdit extends Component {
super( ...arguments );

this.onCreateTable = this.onCreateTable.bind( this );
this.onChangeFixedLayout = this.onChangeFixedLayout.bind( this );
this.onChange = this.onChange.bind( this );
this.onChangeInitialColumnCount = this.onChangeInitialColumnCount.bind( this );
this.onChangeInitialRowCount = this.onChangeInitialRowCount.bind( this );
Expand Down Expand Up @@ -155,16 +154,6 @@ export class TableEdit extends Component {
} ) );
}

/**
* Toggles whether the table has a fixed layout or not.
*/
onChangeFixedLayout() {
const { attributes, setAttributes } = this.props;
const { hasFixedLayout } = attributes;

setAttributes( { hasFixedLayout: ! hasFixedLayout } );
}

/**
* Changes the content of the currently selected cell.
*
Expand Down Expand Up @@ -487,7 +476,6 @@ export class TableEdit extends Component {
} = this.props;
const { initialRowCount, initialColumnCount } = this.state;
const {
hasFixedLayout,
caption,
head,
body,
Expand Down Expand Up @@ -528,7 +516,6 @@ export class TableEdit extends Component {
}

const tableClasses = classnames( backgroundColor.class, {
'has-fixed-layout': hasFixedLayout,
'has-background': !! backgroundColor.color,
} );

Expand All @@ -553,11 +540,6 @@ export class TableEdit extends Component {
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Table Settings' ) } className="blocks-table-settings">
<ToggleControl
label={ __( 'Fixed width table cells' ) }
checked={ !! hasFixedLayout }
onChange={ this.onChangeFixedLayout }
/>
<ToggleControl
label={ __( 'Header section' ) }
checked={ !! ( head && head.length ) }
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/table/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
height: auto;

table {
table-layout: fixed;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this unnecessary since style.scss is loaded in the editor?

Copy link
Member Author

Choose a reason for hiding this comment

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

In testing it was for me, feel free to iterate on PR if you don't find it is in testing @ZebulanStanphill

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @ZebulanStanphill is right, it shouldn't be required.

// Ensure the table element is not full-width when aligned.
width: auto;
}
Expand Down
2 changes: 0 additions & 2 deletions packages/block-library/src/table/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { RichText, getColorClassName } from '@wordpress/block-editor';

export default function save( { attributes } ) {
const {
hasFixedLayout,
head,
body,
foot,
Expand All @@ -26,7 +25,6 @@ export default function save( { attributes } ) {
const backgroundClass = getColorClassName( 'background-color', backgroundColor );

const classes = classnames( backgroundClass, {
'has-fixed-layout': hasFixedLayout,
Copy link
Contributor

Choose a reason for hiding this comment

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

Will have to test whether this change to the save output requires a further block deprecation to be added. Often if it's only a class name change it doesn't seem to require a deprecation.

'has-background': !! backgroundClass,
} );

Expand Down
13 changes: 4 additions & 9 deletions packages/block-library/src/table/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,12 @@

table {
width: 100%;
}

// Fixed layout toggle
.has-fixed-layout {
table-layout: fixed;
width: 100%;
}

td,
th {
word-break: break-word;
}
td,
th {
word-break: break-word;
}

&.alignleft,
Expand Down
9 changes: 2 additions & 7 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,8 @@
background-size: 100px 100%;
// Disable reason: This function call looks nicer when each argument is on its own line.
/* stylelint-disable */
background-image: linear-gradient(
-45deg,
theme(button) 28%,
color(theme(button) shade(20%)) 28%,
color(theme(button) shade(20%)) 72%,
theme(button) 72%
);
background-image:
linear-gradient(-45deg, theme(button) 28%, color(theme(button) shade(20%)) 28%, color(theme(button) shade(20%)) 72%, theme(button) 72%);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this shouldn't have been updated given the comment above.

/* stylelint-enable */
border-color: color(theme(button));
}
Expand Down
1 change: 0 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core__table.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"name": "core/table",
"isValid": true,
"attributes": {
"hasFixedLayout": false,
"caption": "",
"head": [
{
Expand Down
1 change: 0 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core__table__caption.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"name": "core/table",
"isValid": true,
"attributes": {
"hasFixedLayout": false,
"caption": "A table for testing",
"head": [
{
Expand Down
1 change: 0 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core__table__deprecated-1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"name": "core/table",
"isValid": true,
"attributes": {
"hasFixedLayout": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should remain so that the deprecation is tested correctly.

"head": [
{
"cells": [
Expand Down
1 change: 0 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core__table__scope-attribute.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"name": "core/table",
"isValid": true,
"attributes": {
"hasFixedLayout": false,
"caption": "",
"head": [
{
Expand Down
5 changes: 0 additions & 5 deletions packages/e2e-tests/specs/editor/blocks/__snapshots__/table.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ exports[`Table allows adding and deleting columns across the table header, body
<!-- /wp:table -->"
`;

exports[`Table allows cells to be selected when the cell area outside of the RichText is clicked 1`] = `
Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshot shouldn't have been removed as the test is still present. Instead it needs to be updated.

"<!-- wp:table {\\"hasFixedLayout\\":true} -->
<figure class=\\"wp-block-table\\"><table class=\\"has-fixed-layout\\"><tbody><tr><td><br><br><br><br></td><td>Second cell.</td></tr><tr><td></td><td></td></tr></tbody></table></figure>
<!-- /wp:table -->"
`;

exports[`Table allows columns to be aligned 1`] = `
"<!-- wp:table -->
Expand Down
4 changes: 0 additions & 4 deletions packages/e2e-tests/specs/editor/blocks/table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,6 @@ describe( 'Table', () => {
// Create the table.
await clickButton( createButtonLabel );

// Enable fixed width as it exascerbates the amount of empty space around the RichText.
const [ fixedWidthSwitch ] = await page.$x( "//label[text()='Fixed width table cells']" );
await fixedWidthSwitch.click();

// Add multiple new lines to the first cell to make it taller.
await page.click( 'td' );
await page.keyboard.type( '\n\n\n\n' );
Expand Down