Feature slate v0.31.3 #51
Conversation
gabrielrtakeda
force-pushed
the
feature-slate-v0.31.3
branch
from
690519a
to
d1db0a6
Compare
| @@ -1,4 +1,4 @@ | |||
| export * from './SlateEditor' | |||
| export * from './slate-editor' | |||
There was a problem hiding this comment.
It would have been nice for renamed stuff like this to be done in a separate PR since there are already a ton of changes in this it's hard to review it with renames that aren't shown as renames
There was a problem hiding this comment.
Unfortunately to make this separation in a new PR will require a lot of effort and I believe it is not worth ... But it was an excellent point that I need to pay more attention when doing this kind of refactoring...
| @@ -85,7 +85,7 @@ class ImageDataModal extends Component { | |||
| name="openExternal" | |||
| onClick={e => e.stopPropagation()} | |||
| onChange={e => this.setImageAttribute(e, e.target.checked)} | |||
| checked={this.state.imageAttributes.openExternal} | |||
| checked={!!this.state.imageAttributes.openExternal} | |||
There was a problem hiding this comment.
Broken logic ideally would have also been fixed in a separate PR.
| value={value} | ||
| onChange={onChange} | ||
| readOnly={readOnly} | ||
| changeState={changeState} |
There was a problem hiding this comment.
I didn't think changeState existed anymore....?
https://docs.slatejs.org/slate-react/editor
| // From v0.25.3 | ||
| // To v0.31.3 | ||
| // | ||
| migrateStateVersion (value) { |
There was a problem hiding this comment.
If this is going to be common practice for SlateEditor to handle in the future, I think this migration logic should live in its own module and have tests.
Plus, the changes are already not backwards compatible, so why make this part concerned about compatibility?
There was a problem hiding this comment.
@abobwhite this compatibility function is needed because of the state object generated by previous version of slate... We currently save this state object into database and, according to changes on those state objects in this last version of slate, if it was not treated with a compatibility function, like this mentioned on this comment, the state object generated by previous version would break the editor on rendering time...
And, to make a script or a query to normalize all those previous state object on the database, it would require much more effort instead of creating a simple replace function with a regex...
There was a problem hiding this comment.
Thanks for the explanation. I totally understand the need here. But I'm not sure this belongs within slate-editor itself. This seems like something that should be handled by consumers of slate-editor for their specific use-cases. And if you insist that it stay in here, could it be broken out into a separate migration utility? That way people could use it by choice vs automatically happening under the hood? And if you still insist that it must remain under the hood, please break it out into a separate file/class and add some tests. This is definitely something that should not go forward without testing around it. It wouldn't be much work to do that.
There was a problem hiding this comment.
I will decouple this Slate state object compatibility logic from slate-editor's project after this release... I believe it would not impact too much to end users of slate-editor because, if the condition to "migrate" the state object does not match, it will do nothing.
|
@gabrielrtakeda Good work on this. Any thoughts on my review comments? |
|
@gabrielrtakeda My project is needing to update to the latest slate soon for IME support. Any chance we can wrap this up now? It's been out here 2 weeks. I can make adjustments on your branch if you'd like some assistance. |
|
Hi @abobwhite, we're making a big refactoring on our project and we will make a new release soon.. If you need to make some changes, fill free to use my branch and create a new PR to it... |
|
Thanks for the update, @gabrielrtakeda . Do you have any response to my review comments? I think the latter two need to be addressed. |
|
Hi @gabrielrtakeda Our project is nearing a major deadline that I need this to upgrade slate to get a fix for IME. Do you have any timetable for when you might be finishing your testing with the refactoring in your project? Specifically, an ETA to get this merged? |
|
@gabrielrtakeda @lpirola Any update on this? Can I help with something? We're hitting crunch time and need this change to be able to pull in the latest slate for IME fixes |
|
@abobwhite we will delivery a new version next weekend. I guess we will couldn't respond to all your requests. Feel free to propose pull requests to branch |
Related issues