search

liferay / liferay-ckeditor

Other
8 stars 49 forks source link

fix: initialize codemirror with the editor data and update it on data ready #160

Closed NemethNorbert closed 3 years ago

NemethNorbert commented 3 years ago

Hey,

LPS-128735,

These changes will solve the issue on 7.3.x and 7.2.x, on master the issue remains reproducible, however that is more than likely to be caused by data engine, and its still under investigation.

Please review the changes, thanks

/cc @antonio-ortega

antonio-ortega commented 3 years ago

Hi @NemethNorbert ,

Can't we first fix the issue in data engine so we are sure this is properly fixing the original issue in master as well and we know we don't need any extra change?

Thanks.

NemethNorbert commented 3 years ago

Hi @NemethNorbert ,

Can't we first fix the issue in data engine so we are sure this is properly fixing the original issue in master as well and we know we don't need any extra change?

Thanks.

I really wanted to do that, however we did separate the issue within the source plugin and it clearly resolves the issue on or older branches except master. Based on that it seems safe to push this forward and handle the data-engine separately. I pushed this forward as the customer is waiting for 7weeks now, and it's getting urgent to provide some solution. If you don't think it can cause unwanted issues due to the inability to test on master properly, I'd like to move forward with this.

wincent commented 3 years ago

I really wanted to do that, however we did separate the issue within the source plugin and it clearly resolves the issue on or older branches except master. Based on that it seems safe to push this forward and handle the data-engine separately.

Do we have a ticket we can track for the data-engine changes? I'd hate to leave a loose end here that comes back to bite us later because we overlook it. Is it intended that LPS-128735 should remain open until master is fixed?

NemethNorbert commented 3 years ago

I really wanted to do that, however we did separate the issue within the source plugin and it clearly resolves the issue on or older branches except master. Based on that it seems safe to push this forward and handle the data-engine separately.

Do we have a ticket we can track for the data-engine changes? I'd hate to leave a loose end here that comes back to bite us later because we overlook it. Is it intended that LPS-128735 should remain open until master is fixed?

There is no ticket currently opened for the data-engine issue, I will sort that out. My plan is to create a dependancy between the two tickets, we keep LPS-128735 open or under review as QA won't be able to approve it anyway, but we will be able to provide a hotfix with an updated ckeditor to our customers. I think it could be a silver lining if you all agree

julien commented 3 years ago

Closing in favor of #161