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...

CommentFileSizeAuthor
#18 3255077-18.patch4.27 KBjoshahubbers
#17 3255077-17.patch4.41 KBjoshahubbers
#13 interdiff_9_12.txt1.23 KBbeatrizrodrigues
#13 3255077-12.patch4.27 KBbeatrizrodrigues
#9 interdiff-3255077-4_9.txt413 bytesgauravvvv
#9 3255077-9.patch4.27 KBgauravvvv
#4 ckeditor5_typos-3255077-4.patch4.41 KBjoshahubbers
#2 ckeditor5_typo-3255077-2.patch1.23 KBjoshahubbers

Comments

JoshaHubbers created an issue. See original summary.

joshahubbers’s picture

joshahubbers commented
StatusFileSize
new ckeditor5_typo-3255077-2.patch1.23 KB
joshahubbers’s picture

joshahubbers commented
Issue summary: View changes
joshahubbers’s picture

joshahubbers commented
StatusFileSize
new ckeditor5_typos-3255077-4.patch4.41 KB

New patch with both changes, also in the file: core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js

joshahubbers’s picture

joshahubbers commented
Issue summary: View changes
joshahubbers’s picture

joshahubbers commented
Title: Small typo "CKEDitor" » Small typos in ckeditor 5 module.
joshahubbers’s picture

joshahubbers commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Thanks, these all look sensible to me. However, ckeditor5.filter.admin.es6.js doesn't match its transpiled version according to the bot.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3255077-9.patch4.27 KB
new interdiff-3255077-4_9.txt413 bytes

Re-rolled patch #4, Attached interdiff for same. Please review.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
joshahubbers’s picture

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

Ah thanx for the new patch, applies fine at my end.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Small typos in ckeditor 5 module. » Small typos in CKEditor 5 module
Priority: Normal » Minor
Status: Reviewed & tested by the community » Needs work

Nice catches! 🤓

+++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
@@ -60,7 +60,7 @@
-                '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.',
+                '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 plugins that support them.',

While we're changing this, we should then also replace "read only" with "read-only".

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
commented
Status: Needs work » Needs review
StatusFileSize
new 3255077-12.patch4.27 KB
new interdiff_9_12.txt1.23 KB

I applied the small change that you asked @Wim Leers hope it's fine. thank you :)

joshahubbers’s picture

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

I locally applied the patch and it works fine by me!

gauravvvv’s picture

gauravvvv commented
Status: Reviewed & tested by the community » Needs work

Patch is failing custom commands, needs re-roll.

cilefen’s picture

cilefen commented

You 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

joshahubbers’s picture

joshahubbers commented
Status: Needs work » Needs review
StatusFileSize
new 3255077-17.patch4.41 KB

Hm, 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...

joshahubbers’s picture

joshahubbers commented
StatusFileSize
new 3255077-18.patch4.27 KB

Nope, not working. Patch #9 applied without errors. The difference is the 'no newline' part. Try to get that out.

joshahubbers’s picture

joshahubbers commented
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Thanks! :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.3.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 9.4.0

Committed bd122d2 and pushed to 10.0.x. Thanks!

  • alexpott committed bd122d2 on 10.0.x 
    Issue #3255077 by JoshaHubbers, Gauravmahlawat, beatrizrodrigues: Small...

  • alexpott committed d4b608f on 9.4.x 
    Issue #3255077 by JoshaHubbers, Gauravmahlawat, beatrizrodrigues: Small...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Fixed » Patch (to be ported)

Shouldn’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 String change in 9.4.0 tag?

  • xjm committed fbd3adf on 9.3.x authored by alexpott 
    Issue #3255077 by JoshaHubbers, Gauravmahlawat, beatrizrodrigues: Small...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Patch (to be ported) » Fixed
Issue tags: -String change in 9.4.0

This 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!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👍 Thank you!

Status: Fixed » Closed (fixed)

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