Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

fortunatomaldonado
Copy link
Member

#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
Copy link
Contributor

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 fortunatomaldonado changed the title LPS-121472 | Issue 134 fix: add condition for buttoncolor so scrollbar will not be removed Sep 30, 2020
@fortunatomaldonado
Copy link
Member Author

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
Copy link
Member Author

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

@carloslancha
Copy link
Contributor

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!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants