-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Rewrite table block to use a simpler RichText value #8767
Conversation
core-blocks/table/table-block.js
Outdated
plugins: ( settings.plugins || [] ).concat( 'table' ), | ||
table_tab_navigation: false, | ||
} ) } | ||
onSetup={ ( editor ) => this.handleSetup( editor, isSelected ) } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Oh, ignore me, we're still using it in the list block.
I'm seeing these two things: and In master I'm not seeing the initial prompt to choose how many table cells to insert, nor do I see as many inline table commands: That initial prompt, I think, can work well! I think we'll want to style it a little bit, I can probably help there. The chief change being that we'll want to use $default-font and $default-font-size. The extra buttons on the toolbar are fine on the desktop, but how do we make them scale to mobile? I have some ideas for a smart toolbar that pops off items into an overflow menu as space becomes a premium, but in absence of that, perhaps the dropdown was better? Also, I noticed that this control broke: It's broken in |
Regression with fixed width noted in #7899. |
Yes, please! :) Before an earlier rebase this looked better, it would probably be good if controls added inside blocks somehow magically look good (for other block authors).
Sorry I forgot they were inside a dropdown. No problem to change. |
Also as mentioned in the summary I added the prompt to quickly be able to create the right table size needed. This is inspired by the TinyMCE editor table button. Otherwise you'd have to start off with 4x4 and insert many rows and columns. It would also be cool to be able to add more as you type of course. If the prompt is not so good we can take it away, it was just to try out something different. |
I dig the prompt, I think it's a perfect use of the block concept. I also agree it would be nice if block UI were born with the right font and size, any good idea how to do that? I don't necessarily prefer the dropdown, but I think it's probably necessary until such a time as we have a smarter toolbar. |
I'm not sure, but if it requires different styles for the inspector and inside blocks, maybe it requires different components? But it would be good if the same styles work for both. |
979736e
to
abb3228
Compare
There's an error when running
|
First impressions without having looked at the code:
|
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.
This is some great looking code!
I definitely think this refactor makes the table block easier to work with—it should help us a lot with tackling #6923.
value={ initialRowCount } | ||
onChange={ this.onChangeInitialRowCount } | ||
/> | ||
<Button isPrimary onClick={ this.onCreateTable }>Create</Button> |
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.
We should localise this label.
<Button isPrimary onClick={ this.onCreateTable }>{ __( 'Create' ) }</Button>
} | ||
|
||
const classes = classnames( className, { | ||
'has-fixed-layout': hasFixedLayout, |
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.
If we decide to keep it so that rows and columns can't be resized, allowing table-layout: fixed
isn't very useful.
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.
I've left this as it is currently in master. Does it need a change? There's nothing that can be resized atm either.
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.
I didn't realise that we weren't saving the widths and heights in master
. All good then!
icon: 'table-row-before', | ||
title: __( 'Add Row Before' ), | ||
isDisabled: ! selectedCell, | ||
onClick: selectedCell && this.createOnInsertRow( selectedCell ), |
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.
Missing the delta
argument here and on line 164.
onClick: selectedCell && this.createOnInsertRow( selectedCell, -1 ),
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.
Why -1? Inserting one after should be +1 and right before 0?
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.
Yes, you're right, we should pass 0
here. My mistake! 🙂
columnIndex === selectedCell.columnIndex | ||
); | ||
|
||
const id = { |
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.
Minor: I think cell
or cellState
might be a better name since this is what eventually becomes this.state.selectedCell
.
|
||
onCreateTable() { | ||
const { setAttributes } = this.props; | ||
const { initialRowCount, initialColumnCount } = this.state; |
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.
Should we do some bounds checking here? The block seems to break if I enter 0
and 1
as my initial column and row count.
|
||
return ( | ||
<CellTag key={ columnIndex } className={ classes }> | ||
<RichText |
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.
A 10x10 table means we will have 100 instances of TinyMCE loaded on the page. Will these sorts of uses-cases use too much memory?
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.
Yes, sure, we should also be looking into the general performance of this. A table with 100 cells is the same as 100 paragraphs.
label={ __( 'Column Count' ) } | ||
value={ initialColumnCount } | ||
onChange={ this.onChangeInitialColumnCount } | ||
/> |
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.
I think it would feel nice to auto-select this field when the table block is inserted.
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.
Getting the following error when adding autoFocus
:
error The autoFocus prop should not be used, as it can reduce usability and accessibility for users
Should this be something that is part of writing flow instead, that is, always focussing content when a block is created?
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.
Oh. Sure, sounds good 🙂
core-blocks/table/state.js
Outdated
@@ -0,0 +1,104 @@ | |||
/** |
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.
Splitting these functions out is a great idea 🌈
@@ -115,6 +115,11 @@ export function matcherFromSource( sourceConfig ) { | |||
case 'query': | |||
const subMatchers = mapValues( sourceConfig.query, matcherFromSource ); | |||
return query( sourceConfig.selector, subMatchers ); | |||
case 'tag': |
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.
Can we add unit tests for this new attribute source?
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.
There's no tests for the other sources either. Could add tests for all of them though... Not sure if valuable.
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.
My gut says we should have them since this is an API we expose to third party block authors. But if there's none in master
then we can do this separately! 👌
<Section type="head" rows={ head } /> | ||
<Section type="body" rows={ body } /> | ||
<Section type="foot" rows={ foot } /> | ||
</table> | ||
); |
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.
Will we need to add anything to the deprecated
attribute or does this generate the same markup as before?
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.
In there that should still work. I'll test.
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.
So the save value will still look the same. I don't think it needs a deprecation?
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.
Works for me! 👍
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.
This is cool given that is backward compatible 👍
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.
This looks good. As mentioned, these changes seem like it would make the proposed additions to the table block much easier. 👍
As @noisysocks pointed out, I did wonder what the performance implications were in having so many RichText elements. I don't know much about it, but have seen it discussed that we're looking to improve the performance.
I've added a few comments, mostly stylistic things and mostly focusing on the edit file.
setAttributes( { hasFixedLayout: ! hasFixedLayout } ); | ||
} | ||
|
||
createOnChange( { section, rowIndex, columnIndex } ) { |
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.
I think these createX methods would be tidier and simpler if the state and props were only accessed in the returned function. i.e.:
onInsertRow( delta ) {
return () => {
const { section, rowIndex } = this.state.selectedCell;
const { attributes, setAttributes } = this.props;
const newRowIndex = rowIndex + delta;
setAttributes( insertRow( attributes, { section, rowIndex: newRowIndex } ) );
this.setState( { selectedCell: null } );
};
}
It would also mean any issues with the variables being accessed through closure could be avoided, ie. it's always the latest state that is being accessed.
core-blocks/table/state.js
Outdated
@@ -0,0 +1,104 @@ | |||
/** |
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.
Just a naming thing - I'm not sure that 'state' accurately conveys what this file does (it makes me think it stores state).
I feel like something along the lines of helpers
or attribute-helpers
might be an improvement.
I like the pattern of having these functions in an easily testable place, though.
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.
Okay. I don't really care what it's called. It's probably closest to the reducer.js
files we have.
]; | ||
} | ||
|
||
renderSection( { type, rows } ) { |
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.
I think this could just be a separate function component above the class called 'Section'
edit - I see it access this.
a bit, but those things could become props.
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.
Since it contains a RichText
that needs to update attributes, I think it make sense to have a context. Otherwise we can pass attributes
and setAttributes
as wel. I don't mind either way...
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.
That's fair enough. I mostly mention it because I feel like this is an unusual pattern that I've not seen before:
const Section = this.renderSection;
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.
I also think that it would be a good idea to extract this method to its own file and promote it to an independent component. It will decrease the file size of the current edit
implementation and will help to better understand which props are used by those Section
components.
deleteColumn, | ||
} from './state'; | ||
|
||
export default class extends Component { |
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.
Would be good if the class was called TableEdit
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.
Just for my own information, what's the benefit of naming it?
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.
React's warnings and errors mention the component name (based on the class or function name), so it can be really helpful.
icon: 'table-row-before', | ||
title: __( 'Add Row Before' ), | ||
isDisabled: ! selectedCell, | ||
onClick: selectedCell && this.createOnInsertRow( selectedCell ), |
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.
Could also use lodash's partial
or partialRight
to create these partially applied functions, which would mean you could name them onInsertRow
.
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.
As an alternative, might be nice to also look at doing the partial application in the constructor to avoid having to re-apply the partial every render (and it would avoid using lodash).
this.onInsertRowBefore = this.onInsertRow.bind( this, 0 );
this.onInsertRowAfter = this.onInsertRow.bind( this, 1 );
Not sure if that leads to too much misdirection.
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.
Interesting!
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.
You'd still have to create the function though to pass the cell location.
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.
Something like this could be a trade-off which solves both the binding and misdirection issues. It means binding more methods in the constructor, but I think it's easier to read:
insertRow( delta ) {
if ( ! this.state.selectedCell ) {
return;
}
const { section, rowIndex } = this.state.selectedCell;
const { attributes, setAttributes } = this.props;
const newRowIndex = rowIndex + delta;
setAttributes( insertRow( attributes, { section, rowIndex: newRowIndex } ) );
this.setState( { selectedCell: null } );
}
onInsertRowBefore() {
this.insertRow(0);
}
onInsertRowAfter() {
this.insertRow(1);
}
getTableControls() {
return [
{
icon: 'table-row-before',
title: __( 'Add Row Before' ),
isDisabled: ! selectedCell,
onClick: this.onInsertRowBefore,
},
//...
];
}
Anyway, that's just a stylistic thing. I think the bigger issue is the use of a closure to access selectedCell
, which can be avoided given this.state.selectedCell can be accessed directly in those functions. That could be the cause of the bug mentioned by @noisysocks.
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.
plus 💯 to what @talldan shared. In particular, having methods like onInsertRowBefore
and onInsertRowAfter
improves readability of the code by replacing indexes with the description of their functionality.
<Tag> | ||
{ rows.map( ( { cells }, rowIndex ) => | ||
<tr key={ rowIndex }> | ||
{ cells.map( ( { content, tag: CellTag }, columnIndex ) => { |
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 might be easiest if CellTag was derived from the type
, then it reduces the need to store it in attributes. I think the only rule is that it should be a th
in the head, but a td
elsewhere.
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.
You can have th
in tbody
too.
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.
Cheers - I didn't know that.
I moved it to |
abb3228
to
35bca40
Compare
5ec8ca9
to
66ecf75
Compare
Updated with docs and rebased. |
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.
Looks great, and works well in my testing 👍:shipit:
* Browser dependencies | ||
*/ | ||
|
||
const { parseInt } = window; |
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.
/** | ||
* Updates the initial row count used for table creation. | ||
* | ||
* @param {[type]} initialRowCount New initial row count. |
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.
This should be {number}
type :)
@@ -1,30 +1,24 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
|
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.
I don't think we add spaces after dependencies section comment in other places. I don't think we have a rule for that, but sharing as an observation. Personally, I would let prettier
to take care of formatting 😄
deleteColumn, | ||
} from '../state'; | ||
|
||
const table = { |
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.
Can we wrap table
and tableWithContent
with deepFreeze
call to ensure there is no mutation?
@iseulde - I addressed my own feedback. There is one thing I noticed. When no cell is selected, I can see all actions in the dropdown menu, they are active but don't work - which is expected, however, we should update UI to reflect it. |
I did some debugging. |
I think I addressed my previous comment 19b7cb3. It allows to greatly simplify event callbacks, I'm not sure if you like this or not. We can revert part of the changes if you feel like double safety is expected. |
I added one more commit b9c92ed. It fixes the following use case:
Before the patch, they block view was empty. |
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.
This is looking great! I really like the new UX introduced with this PR. Awesome work @iseulde 👏
I applied a few commits, so feel free to review, update, rework them if you don't agree with all my changes.
const { attributes, setAttributes } = this.props; | ||
const { section, rowIndex } = selectedCell; | ||
const { section, rowIndex } = this.state.selectedCell; |
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.
Hm, but selectedCell
can still be null
?
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.
We check it when providing isDisabled
for menu items, then they are disabled. In addition, I added return clause in the event callback.
@@ -1,13 +1,11 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
|
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.
I added these lines because that was @aduth's preference some time ago. I don't really mind, but it is not a doc block... So maybe it's better with the line, or just one asterisk at the start.
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.
If we want to change it, let's update in one batch all of them to match one style.
ca4af7c - yeah, looks good 👍 |
Looks like a bug to me!
|
Description
Split out from #7890 (RichText Structure). The table block uses the
RichText
component for the whole table (many nested elements) in ordere to take advantage of some TinyMCE table controls. This is not really needed: we can create our own table structure with smallerRichText
values, create our own table controls based on that, and drop the TinyMCE table plugin. TheRichText
component is also not really made for such huge fields, only one line of text with formatting, or at most multiple lines with formatting (array of block-like elements).In addition to this, the empty state for the table block shows now some inputs to easily create a table of desired dimensions.
The new format also makes it easier to add more controls later, such as adding a head and foot, and maybe move the controls within table to insert and delete rows and columns. Before we didn't really have that freedom.
How has this been tested?
Ensure the table block still works as before. All controls should be present and work as expected.
Types of changes
Checklist: