See 📌 CKEditor 5 compatibility Needs review
Thanks for the patch!
This is definitely getting close. My upgrade renders some of the icons, but rearranges the content. Some of the icons are lost.
CKEditor prior to upgrade:
<p>test <span class="fontawesome-icon-inline"><i class="fa-solid fa-facebook"></i> </span></p>
<p><i class="fa-solid fa-house fa-2xl"></i> solid<br />
<i class="fa-regular fa-house fa-2xl"></i> regular<br />
<i class="fa-light fa-house fa-2xl"></i> light<br />
<i class="fa-thin fa-house fa-2xl"></i> thin<br />
<i class="fa-duotone fa-house fa-2xl"></i> duotone<br />
<i class="fa-sharp fa-light fa-house fa-2xl"></i> sharp light<br />
<i class="fa-sharp fa-regular fa-house fa-2xl"></i> sharp regular<br />
<i class="fa-sharp fa-solid fa-house fa-2xl"></i> sharp solid<br />
<i class="fa-kit fa-test-custom fa-2xl"></i> custom</p>
CKEditor 5 after upgrade:
<p>
test <span class="fontawesome-icon-inline"><i class="fa-solid fa-facebook"></i></span>
</p>
<p>
solid
<br>
regular
<br>
light
<br>
thin
<br>
duotone
<br>
sharp light
<br>
sharp regular
<br>
sharp solid
<br>
custom<i class="fa-sharp fa-solid fa-house fa-2xl"></i><i class="fa-sharp fa-regular fa-house fa-2xl"></i><i class="fa-sharp fa-light fa-house fa-2xl"></i><i class="fa-regular fa-house fa-2xl"></i><i class="fa-solid fa-house fa-2xl"></i>
</p>Attaching patch
Does it make sense to move that entire module into Font Awesome Icons as a submodule? Since it relies on Font Awesome anyway, I think it makes the implementation a lot cleaner and we don't have to worry in this situation about folks upgrading to CKEditor 5 and losing data if they are on Core versions prior to 10.1 - folks would have to choose to enable this module independently.
Its a little confusing currently for users looking to change to CKEditor5 to have to pull from a separate module.
Would appreciation thoughts on the idea.
Committed. Thank you!
Hmm, I'm not able to reproduce this at all. You've flushed caches, etc. I assume?
Can you upgrade to the dev version and see if this issue persists?
I tracked this down. The iconpicker submodule does not render classes appropriately (has not been updated to use the new fa-solid prefix rather than fas).
Attaching patch - please confirm this fixes your issue
This is strange, I can't reproduce. Are you inserting with an actual icon field, or are you using the CKEditor button?
I suspect this might be due to old data already being saved. If you change the icon to a new one, does it render properly (with the new prefixes)?
Committed.
Committed
Committed. Thank you!
Committed
Tested & confirmed - this is an issue with the determinePrefix function. Attaching patch
Thanks for all your work on this @dk-massive. We will go on a holding pattern until CKeditor's update is available
Committed. Thanks!
Committed. Thank you for the patch!
Partial file configuration is not used if all values are selected (all.js loaded instead), but there are use cases for partial file configuration on CDN
That issue has been resolved and I have pushed a new release.
Committed. Thanks!
I would like to push the release soon, but we need to resolve 🐛 Need backwards compatibility for PHP 7.4 Fixed which is a resultant issue from this patch breaking PHP 7.4 integrations.
This still breaks functionality. The function getIconBaseNameFromClass returns the base name by exploding the class and returning the second argument. The function getIconPrefixFromClass returns the prefix by exploding the class and returning the first argument. In this revised patch, both functions return the same explode argument.
I appreciate the patch. I think something like this would be cleaner
$authenticated_role ? $authenticated_role->grantPermission('access fontawesome additional settings')->save() : NULL;
The patch has to support both. Some folks have PHP 7.4 running an earlier version of Drupal, some have 10.1 and a later version of PHP. If we commit this patch to support the earlier versions, it breaks the later version. We need a solution that supports both.
This patch resolves the compatibility but reverts 🐛 Compatibility with Core 10.1.* Fixed . We need a solution to address both issues
There is no patch attached
This patch is altering the way the application works. Both of the explode functions completely change in functionality with this patch.
Its an issue that needs to be resolved before this can be committed, unfortunately. We can't add CKEditor 5 functionality that just wipes all existing work for users who already have this module functioning that way on CKEditor 4
Committed. Thanks!
This looks great. Merged. Thank you!
Can you provide some info on what the goal of this patch is & steps to review?
Committed. Thank you!
This still needs to be rebased against dev
I've tested this and while it works excellently for inserting new icons, the transition to CKEditor 5 breaks existing content.
Icons which are not wrapped in "fontawesome-icon-inline" span are stripped out of the content on swap to CKEditor 5. To duplicate, you can do something as simple as:
<p><i class="fa-solid fa-house fa-2xl"></i> house</p>
Which works fine in CKEditor 4. When moving to CKEditor 5, the icon is stripped.
Attaching patch
Daniel.Moberly → created an issue.
Before I take a look - I assume you're still working on this & thus the closed merge request?
Merged. Thank you!
Modifying this to include supporting both custom & the new sharp icons, both of which need to be added. Attaching a patch.
You are completely correct. Not sure how that happened. Re-rolling against current dev.
Daniel.Moberly → created an issue.
Tested and confirmed. Will merge shortly. Thank you!
Sure.
Confirmed, this is site breaking and deserves a new release
Merged. Thanks for the fix!
Merged! Thanks!
Patch attached. Committing since the change is minor.
This needs to be rebased against dev please!
Haha, thank you for the work!
I have merged this branch. Thanks!
Looks good - I made an additional commit to disable the color pickers when the "inherit" option is enabled so users don't become confused when their color selections don't work.
Implementing this will force everyone who is currently using Duotone into the new "inherited" feature. I would prefer to leave existing versions alone and allow users to "opt-in" to the inherited option so as not to break existing implementations.
To address the "filter" idea in #5 - this markup is not custom to the Fontawesome module, but rather the actual implementation of Fontawesome itself (see https://fontawesome.com/docs/web/setup/get-started). Changing the way the tags are added prevents folks from using any of the Font Awesome documentation or writing the code themselves site-agnostic.
Its done. Let me know if I can help!
There's something wonky going on with the combination merge requests & patches here. Can you rebase this whole thing into either a git repository or a patch? I can't manage to apply the patch to your repository or to the dev branch