search

liferay / liferay-ckeditor

Other
8 stars 49 forks source link

fix: add condition for buttoncolor so scrollbar will not be removed #135

Closed fortunatomaldonado closed 4 years ago

fortunatomaldonado commented 4 years ago

https://github.com/liferay/liferay-ckeditor/issues/134 https://issues.liferay.com/browse/LPS-121472

A scrollbar will disappear whenever AlloyEditor contains the Text Color button and it is being used.

I found that CKEditor uses this behavior onBlock for its text color block. I added a condition to check for the class cke_ltr which is used only in CKEditor for the Text Color block and not in AlloyEditor. This will prevent any unnecesary changes to CKEditor and will not remove the scrollbar when it is used in AlloyEditor.

Please let me know if there are any questions, comments, or if I need to change any process that I did. Thank you.

carloslancha commented 4 years ago

Hey @fortunatomaldonado thx for the pr!

Could you please remove the autogenerated chore commit update ckeditor build artifacts? We generate them on the release process so we don't need it here.

Also the Semantic PR checker is complaining about the title of the pr, could you please fix it and add a title that describes the fix?

I'll try to take a look and test the pr later. Thx again!

fortunatomaldonado commented 4 years ago

Hello @carloslancha,

Thank you for taking a look at this. I fixed the commit and the title. Please let me know if there is anything else I need to do.

Thank you again!

fortunatomaldonado commented 4 years ago

Hello @carloslancha, Can you provide me any update on this PR? Thank you.

carloslancha commented 4 years ago

Hey @fortunatomaldonado !

I took a look at it. As you discovered, the issue is caused due to different panels behavior in CKEditor and AlloyEditor. CKEditor renders an iframe with the panel inside while AlloyEditor doesn't renders an iframe, so when it gets the body of the document block.element.getDocument().getBody() CKEditor gets the iframe body but AlloyEditor gets the content body.

The thing is that this is a CKEditor plugin and it's working correctly for CKEditor, so introducing changes here to fix stuff in AlloyEditor feels like not the best solution.

It would be reasonable to add some fixes in alloy-editor. Maybe a good starting point could be panel-menu-button

Feel free to reach us via pr or in #t-frontend-infra slack channel if you need further help.

Thx!!