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

Add table block column alignment #16111

Merged
merged 3 commits into from
Jul 31, 2019
Merged

Add table block column alignment #16111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
899c64c
Initial step at adding alignment options in table block edit component
talldan Jun 12, 2019
37a4e06
Make cell alignment work across columns instead of individual cells
talldan Jul 29, 2019
f00dd4e
Address review feedback
talldan Jul 30, 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
12 changes: 10 additions & 2 deletions packages/block-editor/src/components/alignment-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,15 @@ const DEFAULT_ALIGNMENT_CONTROLS = [
},
];

export function AlignmentToolbar( { value, onChange, alignmentControls = DEFAULT_ALIGNMENT_CONTROLS, isCollapsed = true } ) {
export function AlignmentToolbar( props ) {
const {
value,
onChange,
alignmentControls = DEFAULT_ALIGNMENT_CONTROLS,
label = __( 'Change text alignment' ),
isCollapsed = true,
} = props;

function applyOrUnset( align ) {
return () => onChange( value === align ? undefined : align );
}
Expand All @@ -38,7 +46,7 @@ export function AlignmentToolbar( { value, onChange, alignmentControls = DEFAULT
<Toolbar
isCollapsed={ isCollapsed }
icon={ activeAlignment ? activeAlignment.icon : 'editor-alignleft' }
label={ __( 'Change text alignment' ) }
label={ label }
controls={ alignmentControls.map( ( control ) => {
const { align } = control;
const isActive = ( value === align );
Expand Down
15 changes: 15 additions & 0 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 @@ -34,6 +34,11 @@
"type": "string",
"source": "attribute",
"attribute": "scope"
},
"align": {
"type": "string",
"source": "attribute",
"attribute": "data-align"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. since it applied as a class, makes me wonder if at some point, we'd want "classNames" sources. That said, it can be complex and I know it's a deliberate decision to restrict the number of sources available. css @aduth

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we'd considered once before a source serialized as the presence of a class name. Personally I don't have much of an issue with the data- attributes; they can be styled just as well, are a bit easier to parse, and fit well as serving the purpose of a data attribute.

Custom data attributes are intended to store custom data, state, annotations, and similar, private to the page or application, for which there are no more appropriate attributes or elements.

https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes

That being said, I'm not really sure why we bother to serialize this in the HTML at all, if we're not using it for anything like styling? Why not just embed it within the comment delimiters (i.e. omit source)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth As this attribute is in a query, my understanding is that source can't be omitted. A classNames source could be useful in this use case.

An argument against data- attributes is that it's double storing data, but then it's not too much of a problem with the reactive nature of blocks, it's not like there's extra business logic to keep them in sync.

Copy link
Member

Choose a reason for hiding this comment

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

Why use the class at all? Can we not just use the data attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some global styles for text alignment:

gutenberg/packages/block-library/src/style.scss

Lines 150 to 161 in 4ac7dc9

// Text alignments.
.has-text-align-center {
text-align: center;
}
.has-text-align-left {
text-align: left;
}
.has-text-align-right {
text-align: right;
}

Seems better to be able to use those and not have extra styles in the table block.

}
}
}
Expand Down Expand Up @@ -64,6 +69,11 @@
"type": "string",
"source": "attribute",
"attribute": "scope"
},
"align": {
"type": "string",
"source": "attribute",
"attribute": "data-align"
}
}
}
Expand Down Expand Up @@ -94,6 +104,11 @@
"type": "string",
"source": "attribute",
"attribute": "scope"
},
"align": {
"type": "string",
"source": "attribute",
"attribute": "data-align"
}
}
}
Expand Down
169 changes: 126 additions & 43 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 @@ -14,6 +14,7 @@ import {
PanelColorSettings,
createCustomColorsHOC,
BlockIcon,
AlignmentToolbar,
} from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';
import {
Expand All @@ -31,13 +32,15 @@ import {
*/
import {
createTable,
updateCellContent,
updateSelectedCell,
getCellAttribute,
insertRow,
deleteRow,
insertColumn,
deleteColumn,
toggleSection,
isEmptyTableSection,
isCellSelected,
} from './state';
import icon from './icon';

Expand All @@ -64,6 +67,24 @@ const BACKGROUND_COLORS = [
},
];

const ALIGNMENT_CONTROLS = [
{
icon: 'editor-alignleft',
title: __( 'Align Column Left' ),
align: 'left',
},
{
icon: 'editor-aligncenter',
title: __( 'Align Column Center' ),
align: 'center',
},
{
icon: 'editor-alignright',
title: __( 'Align Column Right' ),
align: 'right',
},
];

const withCustomBackgroundColors = createCustomColorsHOC( BACKGROUND_COLORS );

export class TableEdit extends Component {
Expand All @@ -87,6 +108,8 @@ export class TableEdit extends Component {
this.onDeleteColumn = this.onDeleteColumn.bind( this );
this.onToggleHeaderSection = this.onToggleHeaderSection.bind( this );
this.onToggleFooterSection = this.onToggleFooterSection.bind( this );
this.onChangeColumnAlignment = this.onChangeColumnAlignment.bind( this );
this.getCellAlignment = this.getCellAlignment.bind( this );

this.state = {
initialRowCount: 2,
Expand Down Expand Up @@ -156,14 +179,73 @@ export class TableEdit extends Component {
}

const { attributes, setAttributes } = this.props;
const { section, rowIndex, columnIndex } = selectedCell;

setAttributes( updateCellContent( attributes, {
section,
rowIndex,
columnIndex,
content,
} ) );
setAttributes( updateSelectedCell(
attributes,
selectedCell,
( cellAttributes ) => ( { ...cellAttributes, content } ),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a callback here? Can this not be done directly by updateSelectedCell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be overkill, but I feel it's a more useful API. Being able to read the cell's attributes provides an opportunity for doing things like toggling values or incrementing/decrementing values when updating them.

) );
}

/**
* Align text within the a column.
*
* @param {string} align The new alignment to apply to the column.
*/
onChangeColumnAlignment( align ) {
const { selectedCell } = this.state;

if ( ! selectedCell ) {
return;
}

// Convert the cell selection to a column selection so that alignment
// is applied to the entire column.
const columnSelection = {
type: 'column',
columnIndex: selectedCell.columnIndex,
};

const { attributes, setAttributes } = this.props;
const newAttributes = updateSelectedCell(
attributes,
columnSelection,
( cellAttributes ) => ( { ...cellAttributes, align } ),
);
setAttributes( newAttributes );
}

/**
* Get the alignment of the currently selected cell.
*
* @return {string} The new alignment to apply to the column.
*/
getCellAlignment() {
const { selectedCell } = this.state;

if ( ! selectedCell ) {
return;
}

const { attributes } = this.props;

return getCellAttribute( attributes, selectedCell, 'align' );
}

/**
* Add or remove a `head` table section.
*/
onToggleHeaderSection() {
const { attributes, setAttributes } = this.props;
setAttributes( toggleSection( attributes, 'head' ) );
}

/**
* Add or remove a `foot` table section.
*/
onToggleFooterSection() {
const { attributes, setAttributes } = this.props;
setAttributes( toggleSection( attributes, 'foot' ) );
}

/**
Expand All @@ -179,11 +261,11 @@ export class TableEdit extends Component {
}

const { attributes, setAttributes } = this.props;
const { section, rowIndex } = selectedCell;
const { sectionName, rowIndex } = selectedCell;

this.setState( { selectedCell: null } );
setAttributes( insertRow( attributes, {
section,
sectionName,
rowIndex: rowIndex + delta,
} ) );
}
Expand All @@ -202,16 +284,6 @@ export class TableEdit extends Component {
this.onInsertRow( 1 );
}

onToggleHeaderSection() {
const { attributes, setAttributes } = this.props;
setAttributes( toggleSection( attributes, 'head' ) );
}

onToggleFooterSection() {
const { attributes, setAttributes } = this.props;
setAttributes( toggleSection( attributes, 'foot' ) );
}

/**
* Deletes the currently selected row.
*/
Expand All @@ -223,10 +295,10 @@ export class TableEdit extends Component {
}

const { attributes, setAttributes } = this.props;
const { section, rowIndex } = selectedCell;
const { sectionName, rowIndex } = selectedCell;

this.setState( { selectedCell: null } );
setAttributes( deleteRow( attributes, { section, rowIndex } ) );
setAttributes( deleteRow( attributes, { sectionName, rowIndex } ) );
}

/**
Expand Down Expand Up @@ -275,23 +347,28 @@ export class TableEdit extends Component {
}

const { attributes, setAttributes } = this.props;
const { section, columnIndex } = selectedCell;
const { sectionName, columnIndex } = selectedCell;

this.setState( { selectedCell: null } );
setAttributes( deleteColumn( attributes, { section, columnIndex } ) );
setAttributes( deleteColumn( attributes, { sectionName, columnIndex } ) );
}

/**
* Creates an onFocus handler for a specified cell.
*
* @param {Object} selectedCell Object with `section`, `rowIndex`, and
* @param {Object} cellLocation Object with `section`, `rowIndex`, and
* `columnIndex` properties.
*
* @return {Function} Function to call on focus.
*/
createOnFocus( selectedCell ) {
createOnFocus( cellLocation ) {
return () => {
this.setState( { selectedCell } );
this.setState( {
selectedCell: {
...cellLocation,
type: 'cell',
},
} );
};
}

Expand Down Expand Up @@ -351,32 +428,31 @@ export class TableEdit extends Component {
*
* @return {Object} React element for the section.
*/
renderSection( { type, rows } ) {
renderSection( { name, rows } ) {
if ( isEmptyTableSection( rows ) ) {
return null;
}

const Tag = `t${ type }`;
const Tag = `t${ name }`;
const { selectedCell } = this.state;

return (
<Tag>
{ rows.map( ( { cells }, rowIndex ) => (
<tr key={ rowIndex }>
{ cells.map( ( { content, tag: CellTag, scope }, columnIndex ) => {
const isSelected = selectedCell && (
type === selectedCell.section &&
rowIndex === selectedCell.rowIndex &&
columnIndex === selectedCell.columnIndex
);

const cell = {
section: type,
{ cells.map( ( { content, tag: CellTag, scope, align }, columnIndex ) => {
const cellLocation = {
sectionName: name,
rowIndex,
columnIndex,
};

const cellClasses = classnames( { 'is-selected': isSelected } );
const isSelected = isCellSelected( cellLocation, selectedCell );

const cellClasses = classnames( {
'is-selected': isSelected,
[ `has-text-align-${ align }` ]: align,
} );

return (
<CellTag
Expand All @@ -388,7 +464,7 @@ export class TableEdit extends Component {
className="wp-block-table__cell-content"
value={ content }
onChange={ this.onChange }
unstableOnFocus={ this.createOnFocus( cell ) }
unstableOnFocus={ this.createOnFocus( cellLocation ) }
/>
</CellTag>
);
Expand Down Expand Up @@ -467,6 +543,13 @@ export class TableEdit extends Component {
controls={ this.getTableControls() }
/>
</Toolbar>
<AlignmentToolbar
label={ __( 'Change column alignment' ) }
alignmentControls={ ALIGNMENT_CONTROLS }
value={ this.getCellAlignment() }
onChange={ ( nextAlign ) => this.onChangeColumnAlignment( nextAlign ) }
onHover={ this.onHoverAlignment }
/>
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Table Settings' ) } className="blocks-table-settings">
Expand Down Expand Up @@ -502,9 +585,9 @@ export class TableEdit extends Component {
</InspectorControls>
<figure className={ className }>
<table className={ tableClasses }>
<Section type="head" rows={ head } />
<Section type="body" rows={ body } />
<Section type="foot" rows={ foot } />
<Section name="head" rows={ head } />
<Section name="body" rows={ body } />
<Section name="foot" rows={ foot } />
</table>
</figure>
</>
Expand Down
24 changes: 16 additions & 8 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 @@ -40,14 +40,22 @@ export default function save( { attributes } ) {
<Tag>
{ rows.map( ( { cells }, rowIndex ) => (
<tr key={ rowIndex }>
{ cells.map( ( { content, tag, scope }, cellIndex ) =>
<RichText.Content
tagName={ tag }
value={ content }
key={ cellIndex }
scope={ tag === 'th' ? scope : undefined }
/>
) }
{ cells.map( ( { content, tag, scope, align }, cellIndex ) => {
const cellClasses = classnames( {
[ `has-text-align-${ align }` ]: align,
} );

return (
<RichText.Content
className={ cellClasses ? cellClasses : undefined }
data-align={ align }
tagName={ tag }
value={ content }
key={ cellIndex }
scope={ tag === 'th' ? scope : undefined }
/>
);
} ) }
</tr>
) ) }
</Tag>
Expand Down
Loading