sp-client-custom-fields icon indicating copy to clipboard operation
sp-client-custom-fields copied to clipboard

Rich Text has issues when used with OOB Property Pane Fields

Open kmartindale opened this issue 7 years ago • 16 comments

Hey Olivier,

I'm using the Rich Text Property Pane Field, but I'm noticing some issues when I also have a SP OOB PropertyPaneTextField. It looks like updating the oob text field rerenders the property pane entirely. Which creates a new GUID for the ckeditor piece which basically renders it but it wont actually work since the guid has changed. The only fix I've been able to come up with for this is to destroy all instaces of CKeditor before it attempts to render a new one. But this causes a disgusting UI flash as the editor removes and reinstates itself. Have you come across this issue? Is there a different (read better) resolution?

To recreate my issue, simply add an oob text field to a property pane and a custom rich text field. Edit the oob text field and check out the dev console for the error.

Please let me know if you have any questions or ideas!

Thanks, Karissa

kmartindale avatar Jun 26 '17 16:06 kmartindale

Thank you @kmartindale for this bug report. I will investigate it to find a fix. Regards, Olivier

OlivierCC avatar Jun 26 '17 21:06 OlivierCC

Thanks for looking into it.

https://github.com/SharePoint/sp-dev-docs/issues/500

I did find this ^^ since it seemed like not only was the pane refreshing but twice, so looks like that might be a bug fix coming down the line. But my assumption would be that it still would load once triggering the issue all the same. But just thought this might help.

Thanks!

kmartindale avatar Jun 27 '17 13:06 kmartindale

Hi @kmartindale. This bug is fixed in the new version 1.3.6 published today. I fix this issue like that:

  • I removed the guid system and replace it with the use of the 'key' input property (key must be set when you're calling the PropertyFieldRichTextBox custom field)
  • I had specific checks to be sure that the CKEDITOR instance is not already set on the checkbox. As the instance is not destroyed and recreated, there is no flash effects. Olivier

OlivierCC avatar Jun 27 '17 15:06 OlivierCC

Thanks again for this bug report, very usefull for the lib quality ! Best regards, Olivier

OlivierCC avatar Jun 27 '17 15:06 OlivierCC

Hey @OlivierCC I'll check this out! Just off the top of my head, does this account for multiple instances of the webpart on the same page? I had that same thought and I encountered that issue is the only reason I asked :) I'll go see how you implemented it

kmartindale avatar Jun 27 '17 15:06 kmartindale

The key field is different for each PropertyFieldRichTextBox instance. In the code I checked that the instance for this key is loaded or not, so it should work with multiple instances.

OlivierCC avatar Jun 27 '17 16:06 OlivierCC

Hey @OlivierCC I see what you were going for. I'm just not sure its working. Since the new 'guid' is the same for each instance of the same webpart it will break rich text editors on the page since none of them can have the same guid. This was the issue I was having yesterday. I couldn't think of another way around it since it kind of seems like a constraint of the ckeditor plugin. Thoughts?

to replicate: add two of the same webpart to the workbench, on the initialization of the second webpart both rich text editors will break and render as plain textareas.

kmartindale avatar Jun 27 '17 17:06 kmartindale

Hi @kmartindale, I tested today and it's working for me. Be careful to use a different key value for each PropertyRichTextBox value, like it:

PropertyFieldRichTextBox('richTextBox', {
                  label: strings.RichTextBoxFieldLabel,
                  initialValue: this.properties.richTextBox,
                  inline: false,
                  minHeight: 100,
                  mode: 'basic', //'basic' or 'standard' or 'full'
                  onPropertyChange: this.onPropertyPaneFieldChanged,
                  render: this.render.bind(this),
                  disableReactivePropertyChanges: this.disableReactivePropertyChanges,
                  properties: this.properties,
                  disabled: false,
                  onGetErrorMessage: null,
                  deferredValidationTime: 0,
                  key: 'richFieldId'
}),

PropertyFieldRichTextBox('richTextBox2', {
                  label: strings.RichTextBoxFieldLabel,
                  initialValue: this.properties.richTextBox2,
                  inline: false,
                  minHeight: 100,
                  mode: 'basic', //'basic' or 'standard' or 'full'
                  onPropertyChange: this.onPropertyPaneFieldChanged,
                  render: this.render.bind(this),
                  disableReactivePropertyChanges: this.disableReactivePropertyChanges,
                  properties: this.properties,
                  disabled: false,
                  onGetErrorMessage: null,
                  deferredValidationTime: 0,
                  key: 'richField2Id'
 }),

OlivierCC avatar Jun 28 '17 04:06 OlivierCC

Hey @OlivierCC I don't think I was clear. It's not two property pane fields in the same webpart but physically adding two of the same webpart on the workbench. So adding two of your test webparts onto the workbench at the same time.

kmartindale avatar Jun 28 '17 12:06 kmartindale

Humm good point, thanks for this use case. You're right, there may be problems in this case. OK I will try to find a better fix for the next release.

OlivierCC avatar Jun 28 '17 13:06 OlivierCC

Cool I appreciate you looking into it. I'll keep you posted if I come up with a solution. Thanks for all your help!

kmartindale avatar Jun 28 '17 13:06 kmartindale

Hi @kmartindale,

I think that I found solution for this bug. Each webpart as an unique instance ID, available throught the property:

this.context.instanceId

I will change the PropertyFieldRichTextBox signature to provide in entry the web part context. The good solution is to use as id with the key AND the web part context instance ID. With these parameters : each ID will be specify to each web part and we can use two PropertyFieldRichTextBox in 1 web part.

Stay tuned I will publish a new version soon.

Regards,

Olivier

OlivierCC avatar Jun 29 '17 08:06 OlivierCC

Hi @kmartindale,

The fix is now available in the 1.3.7 version. Be careful: the PropertyFieldRichTextBox signature has changed, you can see the updated documentation here https://oliviercc.github.io/sp-client-custom-fields/propertyfieldrichtextbox.

I tested with many different web parts instances in the same page, with multiple rich textbox fields in each web part, and it's working. Please confirm me that it is also the case for you. Thanks again for this bug report,

Regards,

Olivier

OlivierCC avatar Jun 29 '17 10:06 OlivierCC

Hey @OlivierCC you rock. This seems to be working. Thanks so much for listening and taking the matter into your own hands! Closing out.

kmartindale avatar Jun 29 '17 12:06 kmartindale

Hey @OlivierCC just wanted to share a small change I made. In the RichTextBox.ts file where you create the for loop that hooks up the change event. It appeared to work until I attempted to edit the two separate web part instances over and over (testing, you know how it goes), I had to update the loop a bit to be a self invoking function so it properly hooks up the event listeners. I also had to manually update the textareas value since updateElement just wasn't cutting it. Code below if you're interested/want to implement any of it:

//UPDATED FOR LOOP FOR MULTIPLE INSTANCES - KM 7/6 for (var i in CKEDITOR.instances) { let reactHandler = this; (function(i) { CKEDITOR.instances[i].on('change', (elm?, val?) => { ((document.getElementById(reactHandler.key + '-' + reactHandler.context.instanceId + '-editor')) as any).value = CKEDITOR.instances[i].getData(); var value = ((document.getElementById(reactHandler.key + '-' + reactHandler.context.instanceId + '-editor')) as any).value; reactHandler.delayedValidate(value); }); })(i); }

kmartindale avatar Jul 06 '17 15:07 kmartindale

Thanks a lot for this shared code, I will add it in the next release

OlivierCC avatar Jul 06 '17 16:07 OlivierCC