List blocks can now save changes to their content #598
Conversation
|
We should probably update this line https://github.com/WordPress/gutenberg/pull/598/files#diff-059d9a0231c4f69adca5a0f1eb8ba116L238 to match the node where TinyMCE adds the alignment style. Is there a TinyMCE config or getter for this? |
blocks/components/editable/index.js
Outdated
| @@ -329,6 +329,7 @@ export default class Editable extends wp.element.Component { | |||
| } else { | |||
| this.editor.formatter.apply( format ); | |||
| } | |||
| this.editor.setDirty( true ); | |||
There was a problem hiding this comment.
Isn't this done automatically by the formatter calls? Sometimes calling a formatter doesn't change the content (end of text for example) and we may want to avoid this.
There was a problem hiding this comment.
Apparently it isn't. I inspected the dirty state before and after the formatter call and it wasn't being set. This is probably a bug in TinyMCE though.
|
I've updated the pull request with the latest changes on master. After the changes on master it seems that just setting dirty is not sufficient to get it to save state on change of tagName (where the editor must be destroyed and reinitialized). The simplest fix seemed to be to call onChange() before destroying the editor. Also note that the new changeFormats seems to be buggy as the generated ranges are not always valid when it tries to apply them (happens all the time on Firefox and occasionally on Chrome). This can prevent the setDirty call which can prevent saving in onChange however I am not sure what the appropriate fix is. You will probably have to use marker DOM elements to store the selection when formatting is applied. |
| @@ -349,6 +350,8 @@ export default class Editable extends wp.element.Component { | |||
| this.setState( { | |||
| formats: merge( {}, this.state.formats, formats ) | |||
| } ); | |||
|
|
|||
| this.editor.setDirty( true ); | |||
There was a problem hiding this comment.
I can't see why is this needed. I dropped it and it works just as well.
There was a problem hiding this comment.
On Chrome? I confirmed that it seems to work on Chrome too. I'm not sure why because if you throw a log statement in after all the formatting is set then this.editor.isDirty() is false which means in onChange it shouldn't save, however it is saving... One of the other developers here suggested that it might be picking up a node change event and setting the dirty flag later.
On Firefox there are a few problems. Firstly it seems to always throw an IndexSizeError within the call to this.editor.formatter.apply( format ) (apparently it is trying to restore the original selection but the text node that it is trying to select has since been split by the formatting operation and the previous range is not valid). I haven't attempted to fix this in my pull request because it seems out of scope. If I do wrap these problematic calls with a try/catch then as with Chrome the isDirty function still returns false after the formatting has been applied but unlike Chrome it never seems to get corrected so it does not save in the onChange handler.
So the reason I put this.editor.setDirty(true) there is so that this will work when the Firefox IndexSizeError problem has been fixed. Still I suppose that really we just need to get it fixed in TinyMCE.
There was a problem hiding this comment.
I decided to remove the setDirty call since it seems controversial. Note that applying formatting is very buggy/broken on Firefox but I don't know how to address that and it is not really anything to do with the list block.
There was a problem hiding this comment.
I had to re-add setDirty as it was not saving on Chrome after merging the latest changes from master without it.
Note my test procedure was:
- select a word in unordered list
- click bold button
- click button to change to ordered list
|
Notice that the Alignment control is not "toggled" when we click on it (it's not darkened). That's because we're not checking the right parent node for the classname here: if ( tag === 'p' ) {
alignment = node.style.textAlign || 'left';
}The |
|
@youknowriad I will try to find out if there's a way to programatically query this in TinyMCE's API. |
|
Ok, it seems the correct way to do this is to use: I'll see if I can get a working prototype. |
|
I ended up using editor.formatter.matchAll to determine the active formats which fixes the alignment button problem you noted. |
|
@youknowriad Are there any more problems you need me to address? |
blocks/components/editable/index.js
Outdated
|
|
||
| if ( tag === 'p' ) { | ||
| alignment = node.style.textAlign || 'left'; | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Minor: a bit more performant maybe
const link = parents.find( ( node ) = node.nodeName.toLowerCase() === 'a' );
if ( link ) {
formats.link = { value: node.getAttribute( 'href' ), node };
}There was a problem hiding this comment.
I have added your suggested efficiency improvement
|
Related: #567 |
This is an update of the pull request originally submitted by @mimo84 #396
The purpose of the pull request is to:
Note an alternative to changing the list type like we are currently might be to create 2 list types (for unordered and ordered) and provide transformer rules so they appear in the drop down menu. I am unsure which is desired.