search

tinymce / tinymce-blazor

Blazor integration
MIT License
45 stars 14 forks source link

Added correct IDisposable implementation #10

Closed Unlink closed 3 years ago

Unlink commented 3 years ago

Each Blazor component, which uses JSRuntime, especially DotNetObjectReference, should implement IDisposable.

I added IDisposable implementation on .net side and component destroy on js side. There I primary dispose DotNetObjectReference and call remove code on js side.

I'm not sure about the part in js. I added this code to remove the element. I'm not sure if it is needed. When a component is destroyed on .net side, Blazor automatically removes the whole html markup. But maybe there is some JS objects, which needs to be cleaned?

  destroy: (el, id) => {
    if (getTiny() && getTiny().get(id)) {
      getTiny().get(id).remove();
    }
  }

It also fixes a small typo, in CreateScriptLoader which causes multiple requests to tinymce.js when there is more than one editor on-page.

jscasca commented 3 years ago

Hi @Unlink, thank you for this PR!

I'll get some reviews on this. In the meantime, could you sign the CLA so that we can get this merged?

Unlink commented 3 years ago

@jscasca i did not recieved email with CLA code :/

jscasca commented 3 years ago

Hi @Unlink

Sorry for the delay, I tried re-sending the CLA last week but since you didn't replied I assumed you didn't receive it again :(

I tried re-sending it today so just wanted to check with you that it didn't end up in your spam folder by any chance.

Unlink commented 3 years ago

@jscasca i found the issue, there is a typo in mail address in the commit (there is one 9 in addition). What should i do now? Should i close this PR and create new one with correct email address?

jscasca commented 3 years ago

@Unlink if you can do that, it would be amazing! I'll be happy to approve the new PR and get it merged

Unlink commented 3 years ago

I updated current pull request with correct email adress. It is ok now?

jscasca commented 3 years ago

Awesome! I'll get this merged and hope to have a build soon after updating the changelog :)

jscasca commented 3 years ago

This fix was released with v0.0.4

Thanks for the contributions!