When fill="black" is set on the SVG it will break darkmode in Gin. Let's remove it as it will fallback anyway to black if it is not set on the path.
Check out a CKEditor 5 instance with Gin and Darkmode enabled.
Remove fill="black" from SVG paths.
| Before | After |
|---|---|
|
|
|
|
Remove fill="black" from SVG paths.
–
–
–
–
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 3314541-14.patch | 2.61 KB | saschaeggi |
| #4 | Screenshot 2022-10-11 030631.png | 41.78 KB | enaznin |
| ckeditor5_after_darkmode.png | 314.44 KB | saschaeggi | |
| ckeditor5_after.png | 335.75 KB | saschaeggi | |
| ckeditor5_before_darkmode.png | 107.6 KB | saschaeggi |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !2857
Comments
Comment #2
saschaeggiComment #4
enaznin commentedHi,
Ckeditor section is white for me. Turned on the media library and Gin themes dark mode. am I missing something?
Comment #5
saschaeggi@eashika great call, I forgot to mention that you'll need the changes from #3255204: [SUPPORT] CKEditor5
Comment #7
wim leers#3231364: Add CKEditor 5 module to Drupal core introduced
core/modules/ckeditor5/js/ckeditor5_plugins/drupalMedia/theme/icons/medialibrary.svgto core, but it was @zrpnr in https://git.drupalcode.org/project/ckeditor5/-/merge_requests/8/diffs?co... who added it to the CKEditor 5 contrib module in #3198297: Toolbar UI for selecting and sorting buttons.It was never scrutinized since then.
AFAICT it originates from
core/modules/ckeditor/js/plugins/drupalmedialibrary/icons/drupalmedialibrary.png.I don't see any reason we would not do this, because it remains exactly the same visually before and after in Drupal core.
Comment #8
wim leersComment #9
alexpott@saschaeggi can you create a MR - there's no MR here to review or merge with core :)
Comment #10
wim leers🙈🤣
/me just RTBC'd a non-existing patch based on screenshots 😇
Comment #12
saschaeggi@alexpott @Wim Leers I'm not sure on which version this branch here was created but I wasn't able to add a new one. So maybe one of you can cherry-pick the change into a new branch.
Also can we backport this to 9.5.x, too?
Comment #13
wim leers@saschaeggi Can you instead just upload the change as a patch? That's necessary to be able to test against multiple branches anyway. (Otherwise we'd have to open up four merge requests … 🙈)
(I don't know what went wrong in creating this merge request, but it's currently got a whopping 979 commits, which does not make sense.)
Comment #14
saschaeggiMe either, there was already a branch so I guess it was created from an old code revision.
Anyway here is the patch 😊
Comment #15
wim leersConfirmed that the SVG remains identical other than
fill="none"being removed once andfill="black"being removed twice 👍Also confirmed that this still results in an identical looking Media Library toolbar item compared to before in CKEditor 5's default look and feel 👍
Comment #17
saschaeggiThe error in the failed test does not seem to be related to this change
Comment #18
wim leersThat's a random failure. Re-testing.
Comment #19
alexpottConfirmed that the images are the same locally and I agree that the test fails have nothing to do with this issue.
Committed and pushed 78fb66dfd2 to 10.1.x and f2984b45d1 to 10.0.x and 2856c86add to 9.5.x and 53e06d90dd to 9.4.x. Thanks!
ckeditor5 is experimental in 9.4.x so backported all the way there.
Comment #24
saschaeggiAwesome thank you both! 😍