-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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/147 table block (alternate vision) #850
Changes from 23 commits
c3f6fb5
2c1fab0
7eda289
f0fc3cf
462528f
5bc3a62
5d8bec7
172eb3d
fbaf6a9
6347878
55c999a
e591c31
7af4bba
4f8c24c
3ec4f0b
183bb4d
10d42e1
a1826f6
6d1ba0f
c02da21
97c3e9d
e60070b
9a1ef91
3ec9506
bd1a183
ed2d5a5
e4871d3
2249999
a15a781
95c353d
d01937b
7b08fa1
f067183
be1791b
2f6d3e4
9d62f49
6d87fcd
3494440
27b6f01
13fdec5
45ea9ac
fc27514
e9db8b7
79808f6
0404c87
07c5262
b9cdc9d
0cb4cf5
fca185c
dc0dfcf
9daaef9
c133e8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { Fill } from 'react-slot-fill'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { ToolbarMenu } from 'components'; | ||
|
||
export default function BlockMenu( { icon, label, menuLabel, controls } ) { | ||
return ( | ||
<Fill name="Formatting.ToolbarMenu"> | ||
<ToolbarMenu | ||
icon={ icon } | ||
label={ label } | ||
menuLabel={ menuLabel } | ||
controls={ controls } | ||
/> | ||
</Fill> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import './style.scss'; | ||
import { registerBlock, query as hpq } from '../../api'; | ||
import TableBlock from './table-block'; | ||
|
||
const { children } = hpq; | ||
|
||
/** | ||
* Returns an attribute setter with behavior that if the target value is | ||
* already the assigned attribute value, it will be set to undefined. | ||
* | ||
* @param {string} align Alignment value | ||
* @return {Function} Attribute setter | ||
*/ | ||
function toggleAlignment( align ) { | ||
return ( attributes, setAttributes ) => { | ||
const nextAlign = attributes.align === align ? undefined : align; | ||
setAttributes( { align: nextAlign } ); | ||
}; | ||
} | ||
|
||
registerBlock( 'core/table2', { | ||
title: wp.i18n.__( 'Table2' ), | ||
icon: 'editor-table', | ||
category: 'common', | ||
|
||
attributes: { | ||
content: children( 'table' ), | ||
}, | ||
|
||
defaultAttributes: { | ||
content: [ | ||
<tbody key="1"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to address it now, but there's some related chatter in #892 about how to handle or do away with attributes defaulting during initialization. Feel free to leave feedback there if you have thoughts, otherwise we can encounter this when I circle back to that pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will leave this until a decision is made on the case you linked. |
||
<tr><td><br /></td><td><br /></td></tr> | ||
<tr><td><br /></td><td><br /></td></tr> | ||
</tbody>, | ||
], | ||
}, | ||
|
||
controls: [ | ||
{ | ||
icon: 'align-left', | ||
title: wp.i18n.__( 'Align left' ), | ||
isActive: ( { align } ) => 'left' === align, | ||
onClick: toggleAlignment( 'left' ), | ||
}, | ||
{ | ||
icon: 'align-center', | ||
title: wp.i18n.__( 'Align center' ), | ||
isActive: ( { align } ) => 'center' === align, | ||
onClick: toggleAlignment( 'center' ), | ||
}, | ||
{ | ||
icon: 'align-right', | ||
title: wp.i18n.__( 'Align right' ), | ||
isActive: ( { align } ) => 'right' === align, | ||
onClick: toggleAlignment( 'right' ), | ||
}, | ||
{ | ||
icon: 'align-full-width', | ||
title: wp.i18n.__( 'Wide width' ), | ||
isActive: ( { align } ) => 'wide' === align, | ||
onClick: toggleAlignment( 'wide' ), | ||
}, | ||
], | ||
|
||
getEditWrapperProps( attributes ) { | ||
const { align } = attributes; | ||
if ( 'left' === align || 'right' === align || 'wide' === align ) { | ||
return { 'data-align': align }; | ||
} | ||
}, | ||
|
||
edit( { attributes, setAttributes, focus, setFocus } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a new className prop we should add to the root element of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have propagated this prop to the editable. |
||
const { content } = attributes; | ||
return ( | ||
<TableBlock | ||
onChange={ ( nextContent ) => { | ||
setAttributes( { content: nextContent } ); | ||
} } | ||
content={ content } | ||
focus={ focus } | ||
onFocus={ setFocus } | ||
/> | ||
); | ||
}, | ||
|
||
save( { attributes } ) { | ||
const { content } = attributes; | ||
return ( | ||
<table className="blocks-table" style={ { width: '100%' } }> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to serialize either a class name or style in the saved markup? We'll need a front-end/theme styling solution certainly, but this is as of yet mostly unsettled (#422). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I have removed the class and style attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though it seems that all blocks serialize a class name now. |
||
{ content } | ||
</table> | ||
); | ||
}, | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
.editor-visual-editor__block[data-type="core/table2"] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should encourage the use of the new generated classNames for styling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I have moved the table styles so they are a child of |
||
table { | ||
border-collapse: collapse; | ||
} | ||
|
||
td, th { | ||
padding: 0.5em; | ||
border: 1px solid currentColor; | ||
} | ||
|
||
td[data-mce-selected="1"], th[data-mce-selected="1"] { | ||
background-color: $light-gray-500; | ||
} | ||
|
||
&[data-align="left"], | ||
&[data-align="right"] { | ||
min-width: 33%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spaces should be tabs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
max-width: 50%; | ||
} | ||
|
||
&[data-align="left"] { | ||
float: left; | ||
margin-right: $block-padding; | ||
} | ||
|
||
&[data-align="right"] { | ||
float: right; | ||
margin-left: $block-padding; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import Editable from '../../editable'; | ||
import BlockMenu from '../../block-menu'; | ||
|
||
function execCommand( command ) { | ||
return ( editor ) => { | ||
if ( editor ) { | ||
editor.execCommand( command ); | ||
} | ||
}; | ||
} | ||
|
||
const TABLE_CONTROLS = [ | ||
{ | ||
icon: 'table-row-before', | ||
title: wp.i18n.__( 'Insert Row Before' ), | ||
onClick: execCommand( 'mceTableInsertRowBefore' ), | ||
}, | ||
{ | ||
icon: 'table-row-after', | ||
title: wp.i18n.__( 'Insert Row After' ), | ||
onClick: execCommand( 'mceTableInsertRowAfter' ), | ||
}, | ||
{ | ||
icon: 'table-row-delete', | ||
title: wp.i18n.__( 'Delete Row' ), | ||
onClick: execCommand( 'mceTableDeleteRow' ), | ||
}, | ||
{ | ||
icon: 'table-col-before', | ||
title: wp.i18n.__( 'Insert Column Before' ), | ||
onClick: execCommand( 'mceTableInsertColBefore' ), | ||
}, | ||
{ | ||
icon: 'table-col-after', | ||
title: wp.i18n.__( 'Insert Column After' ), | ||
onClick: execCommand( 'mceTableInsertColAfter' ), | ||
}, | ||
{ | ||
icon: 'table-col-delete', | ||
title: wp.i18n.__( 'Delete Column' ), | ||
onClick: execCommand( 'mceTableDeleteCol' ), | ||
}, | ||
]; | ||
|
||
export default class TableBlock extends wp.element.Component { | ||
constructor() { | ||
super(); | ||
this.state = { | ||
editor: null, | ||
}; | ||
} | ||
|
||
render() { | ||
const { content, focus, onFocus, onChange } = this.props; | ||
|
||
return [ | ||
focus && ( | ||
<BlockMenu | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious how you feel about BlockMenu needing to stand on its own merits, versus expanding existing block controls to allow better grouping (spacing between sets of controls) and enable controls to have nested items. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the toolbar-menu component I was trying to replicate the mockups that @mapk and @jasmussen were creating ( #147 (comment) ). It was basically a generalization of the block switcher component with the intent that maybe the block switcher could be changed to share code with it. At the moment the BlockMenu component is a thin wrapper around it but I admit that it has problems. I am learning react as I go (it's not used at Ephox) so I am not very knowledgeable about the slot/fill mechanic. What we probably want to happen is that for multiple uses of the BlockMenu it will add menu items to the menu rather than creating a new menu component each time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't think of a good way to add it to the existing BlockControls but I am happy to take suggestions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have rewritten to use the BlockControls - the downside being that the menu has to be declared after the editor or it appears in the wrong place relative to the formatting menu. |
||
key="controls" | ||
icon="editor-table" | ||
controls={ | ||
TABLE_CONTROLS.map( ( control ) => ( { | ||
...control, | ||
onClick: () => control.onClick( this.state.editor ), | ||
} ) ) } | ||
/> | ||
), | ||
<Editable | ||
key="editor" | ||
tagName="table" | ||
getSettings={ ( settings ) => ( { | ||
...settings, | ||
plugins: ( settings.plugins || [] ).concat( 'table' ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This causes an error to be logged in my console due to missing script:
Looks like we encountered this with the lists plugin after adding local script caching, resolved in #863. We'd probably want to take a similar approach here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have now vendored the table plugin too. Strangely on my Linux mint machine the tinymce and plugins are always served directly (not vendored) which is why it was working for me. My best guess is that it's a permissions problem. |
||
} ) } | ||
style={ { width: '100%' } } | ||
onSetup={ ( editor ) => this.setState( { editor } ) } | ||
onChange={ onChange } | ||
value={ content } | ||
focus={ focus } | ||
onFocus={ onFocus } | ||
showAlignments | ||
className="blocks-table" />, | ||
]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
<!-- wp:core/table2 --> | ||
<table> | ||
<thead> | ||
<tr> | ||
<th>Version</th> | ||
<th>Musician</th> | ||
<th>Date</th> | ||
</tr> | ||
</thead> | ||
<tbody> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2003/05/wordpress-now-available/">.70</a></td> | ||
<td>No musician chosen.</td> | ||
<td>May 27, 2003</td> | ||
</tr> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2004/01/wordpress-10/">1.0</a></td> | ||
<td>Miles Davis</td> | ||
<td>January 3, 2004</td> | ||
</tr> | ||
<tr> | ||
<td>Lots of versions skipped, see <a href="https://codex.wordpress.org/WordPress_Versions">the full list</a></td> | ||
<td>…</td> | ||
<td>…</td> | ||
</tr> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2015/12/clifford/">4.4</a></td> | ||
<td>Clifford Brown</td> | ||
<td>December 8, 2015</td> | ||
</tr> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2016/04/coleman/">4.5</a></td> | ||
<td>Coleman Hawkins</td> | ||
<td>April 12, 2016</td> | ||
</tr> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2016/08/pepper/">4.6</a></td> | ||
<td>Pepper Adams</td> | ||
<td>August 16, 2016</td> | ||
</tr> | ||
<tr> | ||
<td><a href="https://wordpress.org/news/2016/12/vaughan/">4.7</a></td> | ||
<td>Sarah Vaughan</td> | ||
<td>December 6, 2016</td> | ||
</tr> | ||
</tbody> | ||
</table> | ||
<!-- /wp:core/table2 --> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other Table block is assigned to the "formatting" category. Do we want to do the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit strange from a semantic point of view (as we presumably want to discourage people using tables for just formatting as opposed to actual tabular data) however I have made it consistent with the other table.