Problem/Motivation

When the CKEditor module landed, CKEditor did not yet support HiDPI ("Retina") screens. Then, they added support for that. But our custom Image and Link plugins were never updated to support that too.

Worst of all, this is also setting a bad example for D8 contrib, which followed the lead of those two modules, and then also doesn't support HiDPI. See #2625866: CKEditor ships with HiDPI icons for its buttons, Embed doesn't support that?.

Proposed resolution

Just use the APIs that CKEditor provided. And import the images from CKEditor's official plugins. Then the end result looks like this:

Before
After

Remaining tasks

Review.

User interface changes

HiDPI icons for the Image and Link/Unlink buttons.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#13 interdiff.txt670 bytesstar-szr
#8 interdiff.txt893 byteswim leers
#8 ckeditor_drupallink_drupalimage_hidpi-2626570-8.patch10.75 KBwim leers
#5 interdiff.txt1.56 KBwim leers
#5 ckeditor_drupallink_drupalimage_hidpi-2626570-5.patch9.92 KBwim leers
#4 cke_drupallink_drupalimage_hidpi_after.png181.56 KBwim leers
#4 cke_drupallink_drupalimage_hidpi_before.png185.53 KBwim leers
#4 ckeditor_drupallink_drupalimage_hidpi-2626570-4.patch8.41 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Contributed project blocker
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Of course, this is not only about those specific examples, but also about ensuring that the API support is there, as well as documentation.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new ckeditor_drupallink_drupalimage_hidpi-2626570-4.patch8.41 KB
new cke_drupallink_drupalimage_hidpi_before.png185.53 KB
new cke_drupallink_drupalimage_hidpi_after.png181.56 KB

This is so ridiculously simple.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs manual testing
StatusFileSize
new ckeditor_drupallink_drupalimage_hidpi-2626570-5.patch9.92 KB
new interdiff.txt1.56 KB

And now with the updated PHP plugin definitions.

thpoul’s picture

thpoul commented
Status: Needs review » Reviewed & tested by the community

Patch looks great. Manual testing confirms the premise of the proposed resolution and the screenshots of #4.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: ckeditor_drupallink_drupalimage_hidpi-2626570-5.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: -Needs manual testing
StatusFileSize
new ckeditor_drupallink_drupalimage_hidpi-2626570-8.patch10.75 KB
new interdiff.txt893 bytes

Updated test expectation.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs review » Reviewed & tested by the community

Nothing to add. Well done :)

  • Cottser committed 0fa19e0 on 8.2.x 
    Issue #2626570 by Wim Leers, thpoul, DuaelFr: Drupal 8's custom Image...

  • Cottser committed 495fa5b on 8.1.x 
    Issue #2626570 by Wim Leers, thpoul, DuaelFr: Drupal 8's custom Image...
star-szr’s picture

star-szr
he/him
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -196,12 +199,10 @@
           command: 'drupallink',
-          icon: this.path + '/link.png'
...
           command: 'drupalunlink',
-          icon: this.path + '/unlink.png'

eslint didn't like these trailing commas, fixed on commit and pushed to 8.2.x and 8.1.x. Thanks!

core/modules/ckeditor/js/plugins/drupallink/plugin.js
  201:32  error  Unexpected trailing comma  comma-dangle
  205:34  error  Unexpected trailing comma  comma-dangle

✖ 2 problems (2 errors, 0 warnings)
star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new interdiff.txt670 bytes

I meant to include the interdiff of what I changed, here it is.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks Cottser!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.