Saving head and foot values on the state upon toggling #19038
Conversation
abdel-h
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
There was a problem hiding this comment.
Hey @devdivdev Thanks for taking a look at this so quickly. I left some comments.
I'm also adding Needs decision label, as initially it was decided this functionality wasn't required. There's some more info about that here:
#18919 (comment)
| if ( ! isEmptyTableSection( state[ sectionName ] ) ) { | ||
| return { [ sectionName ]: [] }; | ||
| return { [ sectionName ]: [], [ sectionSaved ]: [ ...state[ sectionName ] ] }; |
There was a problem hiding this comment.
You can see here the values returned by toggleSection get saved as attributes:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/table/edit.js#L239
This kind of transient state isn't really the intended use case for the block attributes. I think ideally headSaved and footSaved would instead be stored in component state.
There was a problem hiding this comment.
Yep I noticed that when Travis ran the tests, the attributes object seems to be excpected to be always having the same structure without any extra props.
| return { [ sectionName ]: [], [ sectionSaved ]: [ ...state[ sectionName ] ] }; | ||
| } | ||
|
|
||
| // Check if we already have this section |
There was a problem hiding this comment.
Tiny thing, but part of the coding standards is to have a period at the end of comments
| // Check if we already have this section | |
| // Check if we already have this section. |
| // Check if we already have this section | ||
| if ( state[ sectionSaved ] ) { | ||
| return { | ||
| [ sectionName ]: [ ...state[ sectionSaved ] ], |
There was a problem hiding this comment.
It might be good to also clear sectionSaved state when the data is restored. At the moment the issue following happens:
- Add a table
- Toggle on header rows
- Add some text to the header
- Use the 'Delete row' option to delete the header.
- Toggle the header back on
Expected Result: The header text shouldn't have been restored since it was deleted.
Actual Result: The header text was restored.
talldan
added
[Block] Table
|
Looks like the original issue has been fixed already. Thanks for your efforts here. |
Description
Fixes #18919
How has this been tested?
Screenshots
Types of changes
Checklist: