Problem/Motivation
in core/modules/ckeditor5/js/ckeditor5.filter.admin.js and core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js there is a small typo: "CKEDitor", where the "D" should be undercast.
also the word "tags" at the end of line 45 should be "plugins" i think:
Switching to CKEditor 5 requires, at minimum, the tags "<p> <br>". After switching to CKEditor 5, this field will be read only, and will be updated based on which CKEditor 5 plugins are enabled. When switching to CKEditor 5 from an existing text format with content, we recommend documenting what tags are in use and then enabling the CKEditor 5 TAGS that support them.
I know it is peanuts, but I stumbled on it doing translations...
Patch supplied...
Comment | File | Size | Author |
---|---|---|---|
#18 | 3255077-18.patch | 4.27 KB | JoshaHubbers |
Comments
Comment #2
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #3
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #4
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedNew patch with both changes, also in the file: core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
Comment #5
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #6
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #7
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #8
longwaveThanks, these all look sensible to me. However, ckeditor5.filter.admin.es6.js doesn't match its transpiled version according to the bot.
Comment #9
Gauravvvv CreditAttribution: Gauravvvv at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled patch #4, Attached interdiff for same. Please review.
Comment #10
Gauravvvv CreditAttribution: Gauravvvv at Srijan | A Material+ Company for Drupal India Association commentedComment #11
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedAh thanx for the new patch, applies fine at my end.
Comment #12
Wim LeersNice catches! 🤓
While we're changing this, we should then also replace "read only" with "read-only".
Comment #13
beatrizrodriguesI applied the small change that you asked @Wim Leers hope it's fine. thank you :)
Comment #14
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedI locally applied the patch and it works fine by me!
Comment #15
Gauravvvv CreditAttribution: Gauravvvv at Srijan | A Material+ Company for Drupal India Association commentedPatch is failing custom commands, needs re-roll.
Comment #16
cilefen CreditAttribution: cilefen commentedYou have to recompile changed es6 files like
node ./scripts/js/babel-es6-build.js --file /var/www/html/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
Comment #17
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedHm, I'm a little confused. The file core/modules/ckeditor5/js/ckeditor5.filter.admin.js is automatically compiled in the custom commands I think? When I run the command that cilefen pointed out, I get a lot of changes to the file, like all var and const swapped. In this patch I stripped out all changes except the text-changes, because it can be a node-version thing. I ran yarn install in the core directory and am using node v14.17.3.
But it is not clear to me if we even need to add the compiled file to the patch? Let's try if this one applies well...
Comment #18
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedNope, not working. Patch #9 applied without errors. The difference is the 'no newline' part. Try to get that out.
Comment #19
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #21
Wim LeersThanks! :)
Comment #22
alexpottCommitted bd122d2 and pushed to 10.0.x. Thanks!
Comment #25
Wim LeersShouldn’t this have been committed to 9.3.x too because it’s an experimental module? This particular issue is not very important, but others will be, because it'll give us valuable real-world feedback from early CKEditor 5 adopters. Not committing this patch will increase the number of patch variations we'll have to create in the future 🤓
Or … is that a problem due to the
tag?Comment #27
xjmThis is an allowed change during the beta experimental phase and Gábor Hojtsy also +1ed backporting this from a translation impact vs. disruption perspective, so I cherry-picked this to 9.3.0. Thanks!
We also don't tag individual translation-changing issues per minor anymore since it duplicates l.d.o (and incompletely and without the UX to list translations).
Thanks!
Comment #28
Wim Leers👍 Thank you!