Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CKEditor 5 #6249

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

CKEditor 5 #6249

wants to merge 1 commit into from

Conversation

drbyte
Copy link
Member

@drbyte drbyte commented Feb 24, 2024

Ref #6209

This uses CKEditor 5 "Classic Mode", which turns a textarea field into an in-browser editor field.

This implementation "appears" initially to work, but I haven't tested whether there are features lost by making the change, nor any frustrations that end-users might encounter by using it with Classic mode defaults and no plugins.

Feedback requested because extensive testing is needed, lest content gets accidentally lost or formatting gets messed up because of settings or oversights.

Screen Shot 2024-07-01 at 10 58 55 PM

@drbyte
Copy link
Member Author

drbyte commented Feb 24, 2024

NOTE: Requires changes found in #6248 in order to work correctly.

@drbyte drbyte force-pushed the ckeditor5 branch 2 times, most recently from 8edaf2e to 910625d Compare February 29, 2024 22:38
@scottcwilson
Copy link
Sponsor Contributor

Do you want to merge this into master for eventual release in 2.1? That way people could start testing it more easily?

@barco57
Copy link
Contributor

barco57 commented Jul 1, 2024

we are running behind on this one...
image
this is a ZC 2.0.1

@scottcwilson
Copy link
Sponsor Contributor

We're working on it.

@drbyte
Copy link
Member Author

drbyte commented Jul 1, 2024

Lots of updates today.
Seems to be working for most of the features previously supported in v4.

@drbyte
Copy link
Member Author

drbyte commented Jul 1, 2024

Needs testing for multi-language.

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jul 1, 2024

based on this post here, as well as my experience, the vertical height of the window can no longer be controlled via the config.height setting. it needs to be done via css.

i have added the following line to one of the global stylesheets which seems to work fine.

.ck-editor__editable {
    min-height: 250px;
}

@drbyte
Copy link
Member Author

drbyte commented Jul 2, 2024

@proseLA wrote:
the vertical height of the window can no longer be controlled via the config.height setting. it needs to be done via css.

Thanks for digging that out. I was wondering what direction to take there.

Perhaps we should also set max-height, since it kinda just expands to 100% of the content height, which can be a little unruly. An example in demo data is EZ-Pages number 14.

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jul 2, 2024

i like the idea of having both (min-height and max-height) of those there. and 250 and perhaps 500 for max could be good starting points. but whatever seems right to others is fine with me...

as to which css file you want to include them in... well i have no opinion on that one...

@drbyte
Copy link
Member Author

drbyte commented Jul 2, 2024

Tested on versions v210, v200, v158, v157, v156 to confirm that the CKE5 editor loads correctly using the code changes in this PR.

/admin/includes/ckeditor.php (whole file)
/admin/includes/css/stylesheet.css (just the last few lines related to .ck-editor__editable must be copied into the old file)
/editors/ckeditor5/config.js (whole file and directory)

(Note on v156 the stylesheet is in the includes dir, not in the css subdir)

@drbyte
Copy link
Member Author

drbyte commented Jul 2, 2024

NOTE: I found some issues with the multi-language part. Will explore more tomorrow.

@drbyte
Copy link
Member Author

drbyte commented Jul 2, 2024

For english-only stores, the code currently in this PR should work fine on v156-thru-v201

@matt-in-real-life
Copy link

For english-only stores, the code currently in this PR should work fine on v156-thru-v201

In Chrome 126.0.6478.116, CKEditor fails to initialize (remains as default textarea) and see this in the console:

TypeError: Reduce of empty array with no initial value at Array.reduce (<anonymous>) at translation-service.ts:239:16 at new Ea (locale.ts:132:23) at new Qa (context.ts:163:17) at new Cg (editor.ts:293:37) at new <anonymous> (elementapimixin.ts:23:2) at new vC (classiceditor.ts:56:3) at classiceditor.ts:192:19 at new Promise (<anonymous>) at vC.create (classiceditor.ts:191:10)

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jul 3, 2024

For english-only stores, the code currently in this PR should work fine on v156-thru-v201

In Chrome 126.0.6478.116, CKEditor fails to initialize (remains as default textarea) and see this in the console:

TypeError: Reduce of empty array with no initial value at Array.reduce (<anonymous>) at translation-service.ts:239:16 at new Ea (locale.ts:132:23) at new Qa (context.ts:163:17) at new Cg (editor.ts:293:37) at new <anonymous> (elementapimixin.ts:23:2) at new vC (classiceditor.ts:56:3) at classiceditor.ts:192:19 at new Promise (<anonymous>) at vC.create (classiceditor.ts:191:10)

i have english only test stores, running in the same chrome version, and am unable to reproduce the error.

do you have any more info? javascript versions?

@matt-in-real-life
Copy link

do you have any more info? javascript versions?

Zen Cart v1.5.8a
jQuery 3.6.1
jQuery UI 1.13.2

@njcyx
Copy link
Contributor

njcyx commented Jul 3, 2024

For english-only stores, the code currently in this PR should work fine on v156-thru-v201

In Chrome 126.0.6478.116, CKEditor fails to initialize (remains as default textarea) and see this in the console:

TypeError: Reduce of empty array with no initial value at Array.reduce (<anonymous>) at translation-service.ts:239:16 at new Ea (locale.ts:132:23) at new Qa (context.ts:163:17) at new Cg (editor.ts:293:37) at new <anonymous> (elementapimixin.ts:23:2) at new vC (classiceditor.ts:56:3) at classiceditor.ts:192:19 at new Promise (<anonymous>) at vC.create (classiceditor.ts:191:10)

Greetings. I have the exact same error as @matt-in-real-life mentioned. ck5 didn't load.

Zen Cart v1.5.8a
jQuery 3.6.1
jQuery UI 1.13.2
PHP 8.1
English only site.
Response classic template.

Chrome 126.0.6478.127

I downloaded two files from the "Files Changed" tab on the top of this page. Code has been manually added to the end of the css file.

@drbyte
Copy link
Member Author

drbyte commented Jul 3, 2024

Hmmm. I can't duplicate the problem on Chrome 126.0.6478.127 either. Also tried in incognito mode with no plugins/extensions enabled.

While it shouldn't matter, do you mind listing what plugins/extensions you have installed and enabled in Chrome?

@proseLA
Copy link
Sponsor Contributor

proseLA commented Jul 3, 2024

i am not able to reproduce it as well...

@drbyte
Copy link
Member Author

drbyte commented Jul 3, 2024

Puzzled that it's even accessing translation-service.ts ... but haven't dug into that yet. Just observation from error msg posted.

@scottcwilson
Copy link
Sponsor Contributor

@matt-in-real-life Can you test from another browser in case it's related to an extension you have enabled ?

@lat9
Copy link
Contributor

lat9 commented Jul 3, 2024

Could that just be the browser-caching the javascript?

@matt-in-real-life
Copy link

Same issue in Incognito in Chrome and in FireFox 115.12.0esr (64-bit) with no extensions. All on macOS 13.6 (22G120).

Could that just be the browser-caching the javascript?

Cache cleared after making the changes initially, but which JS are you referring to? The relevant config.js is a new file in a new directory, so won't be cached.

@drbyte
Copy link
Member Author

drbyte commented Jul 4, 2024

Thanks for your patience.
I was able to trigger the error.

It appears that it doesn't like having a translations configuration section if it's empty.

I've pushed an update which seems to work fine now with single-language sites. Tested on v158 and v210.

On multiple-language sites it also works, but ... Multi-Language works now.

@drbyte
Copy link
Member Author

drbyte commented Jul 4, 2024

@matt-in-real-life @njcyx You can try again using the updated /admin/includes/ckeditor.php file.

@drbyte
Copy link
Member Author

drbyte commented Jul 4, 2024

Update: Multi-language should be working correctly now as well.

@drbyte
Copy link
Member Author

drbyte commented Jul 5, 2024

Undecided about removing Subscript,Superscript from the default button bar options. It'd make it cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants