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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JoshaHubbers created an issue. See original summary.

JoshaHubbers’s picture

JoshaHubbers’s picture

Issue summary: View changes
JoshaHubbers’s picture

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

JoshaHubbers’s picture

Issue summary: View changes
JoshaHubbers’s picture

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

Issue summary: View changes
longwave’s picture

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

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

Gauravvvv’s picture

Status: Needs work » Needs review
JoshaHubbers’s picture

Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

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

Status: Needs work » Needs review
FileSize
4.27 KB
1.23 KB

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

JoshaHubbers’s picture

Status: Needs review » Reviewed & tested by the community

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

Gauravvvv’s picture

Status: Reviewed & tested by the community » Needs work

Patch is failing custom commands, needs re-roll.

cilefen’s picture

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

Status: Needs work » Needs review
FileSize
4.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

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

alexpott’s picture

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

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

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

👍 Thank you!

Status: Fixed » Closed (fixed)

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