-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 tinymce on-demand loading #18105
base: main
Are you sure you want to change the base?
Conversation
92602a4
to
d70ed2a
Compare
c8015f1
to
a8057d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See @AdrienClairembault comment
7000ad9
to
fc44b0c
Compare
fc44b0c
to
5ee22ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a4912d1
to
b6fa363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to now work for some placeholders but not all:
In this example the second one doesn't have the correct color.
Padding issues seems to be mostly resolved, there is still some additional left padding that seems to be applied when compared to main
but I can live with it.
Nevertheless, maybe it mean the styles are not fully charged so it might be worth investigating because there might be other issues that we may miss here.
8afbf96
to
2139d64
Compare
I've reverted back to the oxide skin to unblock this PR. I'll work on fixing it in a different PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are still failing.
63bdf23
to
5d79354
Compare
Checklist before requesting a review
Description
TinyMCE is currently loaded in two ways:
Neither of these cases are very good.
The first case technically works, but there can be some cases where the library is loaded for no reason. For example, it was included for the
central
page but its only ever actually used if you click on an event in the planning widget which loads the Reminder/Event form in a modal. If you don't actually initialize an editor, you have a 680kb script (non-minified size) being loaded for nothing.The second case makes more sense, except that it doesn't actually load the library if initialized any time after
includeHeader
is called becauseinitEditorSystem
callsrequireJs
. Instead, it adds the library to the session property and would have it automatically load the next timeincludeHeader
is called (page refresh). This is how you end up with bugs like #3663. The solution for the referenced issue was to just load the library for the entirecentral
sector which is more of a workaround than addressing the actual issue.You could directly import the required scripts by adding
script
tags with thesrc
attribute set insideinitEditorSystem
, but that would load them multiple times if there are multiple editors.The solution here is to move the initialization into its own script which imports the extra JS files and then use dynamic imports to ensure the module is only loaded once but still allows initializing multiple editors.