πŸ‡ΊπŸ‡ΈUnited States @tim.tripp

Account created on 16 February 2023, over 1 year ago
#

Recent comments

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Tab key navigation works in the latest commit.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I'm interested in this as well as another CKEditor 5 plugin developer, it was brought to my attention by @SnowCoder that the advanced link attributes are getting stripped from my Icon widgets as well as Drupal Media widgets. Drupal Media seems better than Icon at preserving the target="_blank" specifically but always strips the other attributes like title and aria-label just like Icon.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Drupal Media seems better than Icon at preserving the target="_blank" specifically but always strips the other attributes like title and aria-labeljust like Icon. Not quite sure what's going on with their solution to this.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Great, can you open a separate issue for that one? I'll merge into the dev branch and mark this one as resolved as it relates to the multiple FA classes bug. Thanks for finding that one.

It looks like Drupal Media exhibits the same behavior of stripping the advanced link attributes so the issue isn't unique to icons. You may even try opening an issue in the Editor Advanced Link module to say that both the Drupal Media and Icon plugins are stripping the advanced link attributes when they are linked, maybe the contributors there can get to the bottom of what's happening.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I need to know how you're seeing "Advanced Links" because if you're using a contrib module to mess with links this is likely why I can't reproduce your issue with the links. I don't have "Advanced Links" on my installation. Also are you saying it just strips the attributes now, or is it still the entire link?

Thanks for verifying the fa-fw bug is fixed.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Are you using a third-party contrib module for "Advanced Links"? The stock Link plugin doesn't seem to provide that.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I can produce the fa-fw problem but not the stripping of links in 1.1.0. The only causes I can think of for that are a caching issue or the Link plugin isn't enabled in your editor (a change from 1.0.0 was the Link plugin must be enabled to allow linkHref). How are you allowing target="_blank" on your links?

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Fix pushed to dev branch, working on my end with both 41.2.0 and 41.3.1 but needs further review.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Appreciate you reviewing @matthiasm11! Marking as resolved.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Hi @dpi, please verify whether this patch stops the attribute loss.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I'll look into ways to make the editor see them as separate, like it does when there's more than just the icon inside the link, but I'm afraid of causing other issues when messing with how the editor sees links. Right now the way to mitigate this would be to put something else inside the link so it's not just the icon.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

This kind of issue is difficult to fix because of the way CKEditor 5 handles links. The entire <a href="http://google.com" class="myclass"><i class="fa-brands fa-accusoft fa-7x">&nbsp;</i></a> is seen by the editor as a single widget, an icon with a linkHref attribute. Because the link is only an attribute of icon and not an object of its own, I don't actually know how I can attach classes to it. I can possibly add a linkClasses attribute to the icon but that doesn't solve the problem of other attributes like target="_blank" that may exist on the link still getting discarded.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Another option would be to use hook_page_attachments_alter to remove the library from attachments for anonymous users.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Your use case is unique as most will be wanting to load the library for anonymous users also to use the icons in content. You could use an admin sub-theme to restrict it to the admin theme only.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

The icons will generally also inherit whatever the current text color is.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I put in the ability to make your own CSS classes and extension plugin that adds functionality to the icon toolbar. We opted to do this for coloring icons with our brand-specific color scheme.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

@ari.saves Thanks for reviewing! Fix has been committed to the dev branch.

The category information is in categories.yml, if you want to modify them with your custom icons and categories you can edit this file. You can also easily add custom icons to the recommended category.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

You'll have to enable the "Custom" style in the settings for your text format.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Can you provide the CSS class names of the custom icon style? I need the equivalent of fas and fa-solid class names for the solid icon style.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

@ari.saves Thanks! I'd like to close this issue about the JavaScript error, feel free to open a separate one for the custom icons not showing up in the picker and I'll credit you once a fix is available for that.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

New patch to make TypeScript expect either a string or number instead of string only, let me know if this works.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Yep that'll do it. The Font Awesome module has no check to ensure these are all strings, and doesn't trim them either (which likely breaks some terms for the module's own autocomplete widget, I've seen a bunch that needed trimming which is why I added that). Since this module is pulling the metadata directly from the Font Awesome module when "Custom" gets selected, the issue gets passed down the pipeline.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

They may have introduced a bug into their metadata YAML file, but it's something we can account for. Do you have the section of their YAML file that's breaking it?

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

undefined is okay in some situations, the index parameter is optional and predictably it'll just append to the end if not specified. I'll go ahead and merge this in, thanks!

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Here's a patch to fix the error in the OP. The error is triggered whenever the "Expand" button is visible on the initial opening of the icon picker (Font Awesome Free has fewer icons and wasn't triggering this), so good catch on that one.

"e.trim is not a function" is unrelated and shouldn't happen unless your Font Awesome metadata is somehow corrupted, or maybe there's a bug in your specific version of the metadata. I don't have access to your custom Font Awesome Pro metadata so more information would be helpful, such as the value of `e` (the specific search term that's not a string), and which icon it's breaking on. Because this is an open-source project, you can use `npm run watch` to get a more verbose copy of the script for setting breakpoints / other debugging purposes.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Adding it with no index works but that unfortunately breaks the focus tracker item order. The index is there to ensure the focusable the elements are in the correct order when using keyboard-based navigation.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

@esch Updated patch should enable an icon to be inserted inside the existing link, please verify this is the case.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

If there's no text after the icon then I do have to be more particular about how I select it in Safari than Firefox.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Interesting. I can't reproduce this in my version of Safari (16.6), selecting text along with an icon and linking everything seems to work fine.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Which version of Drupal / CKEditor are you on? I remember there were some bugs (including a Safari-specific bug I'd encountered) with CKEditor 5 that were fixed, hence why I recommend Drupal 10.1.3 as the minimum version.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

You have to use the arrow keys to place the cursor after a widget that's the last element in the editor, between a widget and a newline, or between two widgets that aren't separated by any text. It seems like that's a CKEditor behavior as in my testing it happens with other widgets as well. Might be a good issue to open with the CKEditor project as I wish it worked a little bit better as well.

Inserting a new icon in the middle of existing linked text does break the link apart, I'll see if I can fix this. Selecting an existing icon along with some text and linking all of it with a new link works fine in my testing.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

@esch or @kevinkrafts please review this patch. Hoping this will be sufficient to fix the issue.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Registering an event listener and calling event.stop() should disable the link balloon from showing on click. This is fine as long as the link balloon can still be shown via the link button in the main CKEditor toolbar.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Turned out to be more trivial than I expected – simply allowing the linkHref attribute on icons did the trick. The remaining issue is the link balloon clashing with the icon toolbar which may not be as trivial to fix.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Thanks for reviewing @siemen_hermans. PR has been merged into the dev branch. Marking as resolved.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I don't have a multilingual site to test this, so if you can review my changes to the issue fork that would be greatly appreciated.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I amended the commit, you should have credit now.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

My bad, I'm still new to Drupal.org and how things work here. I also accidentally merged this without editing the commit message to mention the issue number which I had intended on doing.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I merged in your work, thanks for the help Shreya!

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Hi Shreya, it was a very easy fix so I pushed it up myself. Please run phpcs again to verify that it still passes.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

Hi Shreya, thanks for doing this. Some readability was lost in the create methods of all the classes. Please indent it so that, for example,

return new static(
$container->get('module_handler'),
$container->get('ckeditor5_icons.CKEditor5Icons')
);

becomes

return new static(
  $container->get('module_handler'),
  $container->get('ckeditor5_icons.CKEditor5Icons')
);

in all the classes and run phpcs again. A few of the comment lines in MetadataController.php (lines 70, 86, 102) were mistakenly altered as well and should be reverted. Otherwise this looks good and I will merge it after the fixes.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

If other folks would like to pitch in to help with CKEditor 4 backward-compatibility I'd certainly welcome that though! Just have no way of testing that on my end.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

I did the separate module because we only actually need the CKEditor 5 plugin and nothing else – hence why the plugin is self-contained and the dependency is optional if using the built-in metadata and attaching your own library. I didn't take into account CKEditor 4 backward-compatibility concerns as in CKEditor 4 we were using a Shortcode for icons and have a different migration plan for that. I recommended Drupal 10.1.3 or later on the project page but inserting icons works in 9.5 as well (it inserts a &nbsp; so the element isn't technically empty).

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

We needed this as a CKEditor 5 plugin so we went ahead and did our own version. The project is CKEditor 5 Icons β†’ if anyone is interested.

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

If you're having problems with csrf_token in a local environment then turn off asynchronous metadata. I can add a troubleshooting section to document this better and maybe an option to turn off csrf_token without completely disabling asynchronous metadata.

However this error isn't just because of using a local environment, in my local environment it doesn't happen. Do you have your environment set up specifically to allow access to CKEditor without first logging into a user account?

πŸ‡ΊπŸ‡ΈUnited States tim.tripp

tim.tripp β†’ made their first commit to this issue’s fork.

Production build 0.69.0 2024