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: Restore htmlwriter to have beautification in our source view #152

Merged
merged 3 commits into from Feb 15, 2021

Conversation

NemethNorbert
Copy link
Member

Hey,

LPS-126202
By restoring htmlwriter, the beautification issue is resolved in AlloyEditor.

This issue is not reproducible with ckeditor at the moment as we used js-beautify to beautify the html when switching to source mode in our new source editor plugin -> LPS-118290

Restoring the htmlwriter plugin resolves the issue in AlloyEditor and it's a battle tested solution we used for years.

Please review my changes, thanks

@NemethNorbert NemethNorbert changed the title Fix: Restore htmlwriter to have beautification in our source view fix: Restore htmlwriter to have beautification in our source view Feb 10, 2021
@julien
Copy link
Contributor

julien commented Feb 10, 2021

Hi @NemethNorbert, thanks for sending this contribution.

This sounds like an interesting idea. I was wondering if we couldn't get rid of js-beautify in the codemirror plugin since this is going to be enabled.

@NemethNorbert
Copy link
Member Author

Hey @julien,

I did not test that, but I'd believe we should be able to just do that

@julien
Copy link
Contributor

julien commented Feb 10, 2021

Let me fetch this branch and give it a try.

@julien
Copy link
Contributor

julien commented Feb 10, 2021

@NemethNorbert here's what I see.

The difference is minimal, but still worth talking about (in my opinion)

With html_beautify (i.e. right now)

With htmlwriter (i.e. these changes)

As you'll see the "line height" is bigger with htmlwriter, I don't know if that's an issue but that's the only visible change I saw.

In terms of "size" changes, when running the default build (i.e. minified):

  • With html_beautify, the size of the ckeditor/ckeditor.js script is 600K
  • With htmlwriter, the size of the ckeditor/ckeditor.js script is 604K
    (I edited the files to make sure html_beautify isn't included)

I expected it to be the opposite, so you might want to re-check that.

In any case, we might want to leave this open for discussion, because in terms of look and feel it does change slightly, so I'll ping @drakonux to have his opinion too.

@jbalsas
Copy link
Contributor

jbalsas commented Feb 10, 2021

If we can get rid of an external dependency and use something ootb, I think that's desirable, even if the behaviour is slightly different.

@julien
Copy link
Contributor

julien commented Feb 10, 2021

@jbalsas yeah that's true, the fact that htmlwriter is something that we get ootb is an advantage.

@NemethNorbert
Copy link
Member Author

NemethNorbert commented Feb 10, 2021

@julien I'd say in this case its better to ship our ckeditor with htmlwriter included, as this is the same behaviour they would see in other ckeditors as far as I understand. Having said that, I would prefer the experience provided by js-beautify as a user.

On the other hand htmlwriter is configurable, I think we should be able to remove the extra lines between the html elements

@julien
Copy link
Contributor

julien commented Feb 10, 2021

@NemethNorbert you're right, by configuring it, we should be able to get more or less the same look and feel so I guess we're covered.

Since we all agree that this is the good direction, would you be able to disable/remove html_beautify from the codemirror plugin or do you want us to take care of that?

I thought that this could be done as part of this PR, but it doesn't have to.

@NemethNorbert
Copy link
Member Author

Hey @julien,

I can remove it, but it would be better if we also provide the configuration for htmlwriter when we do that. Should we do all that on this PR?

@julien
Copy link
Contributor

julien commented Feb 10, 2021

@NemethNorbert yes that sounds even better, if you think it's possible.

@NemethNorbert
Copy link
Member Author

@NemethNorbert yes that sounds even better, if you think it's possible.

@julien sure, I have to wrap up some other tasks today, but after that I will get right back to this one and see what I can do.

Cheers

@julien
Copy link
Contributor

julien commented Feb 10, 2021

@NemethNorbert no problem, and if you need help don't hesitate - thanks!

@NemethNorbert
Copy link
Member Author

Hey @julien,

I updated the PR with the removal of js-beautify. I also went ahead and tried to configure the htmlwriter to remove those extra line breaks, but could not make it work yet. I'd like to suggest to move ahead with this PR as is, and work on the htmlwriter configuration on a separate ticket. The resolution of the AlloyEditor source issue is getting more and more urgent on the support side and we can't wait more. Thank you

@julien
Copy link
Contributor

julien commented Feb 15, 2021

Hi @NemethNorbert

Thanks. Yes that sounds reasonable. Let me just re-fetch and test to double check and I'll merge if it's OK.

@julien
Copy link
Contributor

julien commented Feb 15, 2021

LGTM

@julien julien merged commit e9dcc50 into liferay:master Feb 15, 2021
julien pushed a commit to julien/liferay-portal that referenced this pull request Feb 15, 2021
**Changes**:

- fix: Restore htmlwriter to have beautification in our source view
([\#152](liferay/liferay-ckeditor#152))

[Full changelog](liferay/liferay-ckeditor@v4.14.1-liferay.16...v4.14.1-liferay.17)
brianchandotcom pushed a commit to brianchandotcom/liferay-portal that referenced this pull request Feb 15, 2021
**Changes**:

- fix: Restore htmlwriter to have beautification in our source view
([\#152](liferay/liferay-ckeditor#152))

[Full changelog](liferay/liferay-ckeditor@v4.14.1-liferay.16...v4.14.1-liferay.17)
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.

None yet

3 participants