Split up ckeditor.admin.js

Anything else i missed?

Hi Wim,

I'm at the PNW Drupal Summit core sprint and was just about to try re-rolling this patch, but noticed you just requeued for testing about 40 min. ago. and wanted to make sure i wasn't duplicating work. If you working on it now, let me know and i'll tackle something else.

Rerolled against HEAD. No other changes.

I don't know how you rerolled, but unfortunately the reroll is wrong. The changes introduced by the last commit, for example, are lost.

When rerolling a patch that is splitting up a file into multiple files, you must make sure to include any new changes in the additions, rather than only updating the deletions to allow the patch to be applied.

Since this is a JS-only patch, it doesn't matter what testbot says: we have no JS test coverage, hence a green patch doesn't mean anything, unfortunately.

Could you reroll this again? Thanks!

registerGroupMove() and openGroupNameDialog() still need the same treatment as registerButtonMove() already received: they need to be moved onto the Drupal.ckeditor object. Then this should be good to go! :)

registerGroupMove() and openGroupNameDialog()
changes added to patch
interdiff.txt between patch of comment 15 en 18

Works perfectly.

But:

1. at the end of Model.js, there was no empty line. So, added that.
2. the new files had 4-space indentation, rather than 2-space indentation. Fixed that.

Here is the result of git diff -w from 18 to 20.

Evaluating this normal task per https://www.drupal.org/contribute/core/beta-changes

Updating the issue summary.

Note, according to the allowed changes handbook, this maybe should be postponed till 8.1.x

Could it reduce fragility? Or maybe improve performance (maybe it loads only the js that is needed)?

----

I read the related issues.. in #2162407: Split up toolbar.js the motivation is:
it is confusing to read.

I'm not sure I see the point of this issue. At least if this issue motivation is more than that, the summary should be updated.

thx Wim
So will the patch be submitted?

#24: You've already submitted/posted the patch. The patch is now marked as "Reviewed & tested by the community", which means that the next step is for a Drupal 8 core committer to do a final review and if no problems are found, commit it.

The problem with this patch is the whilst I can see the benefits - I'm not sure they outweigh the risks - we have no test coverage of javascript patches. It would be good to see evidence of manual testing. I think it is worth trying to move to a javascript documentation standard and if this unblocks #2182153: [Meta] Document Drupal JavaScript using JSDoc then ok - but test evidence please.

I'm sorry, I didn't mention it, but I did extensive manual testing prior to RTBC'ing in #20.

Together with Jesse Beach, I was responsible for getting this into Drupal 8, and precisely because it is the result of *so* many hours of development, to make the UX and accessibility of configuring CKEditor awesome, I would never RTBC a patch like this if I weren't certain it didn't cause any problems. I thoroughly tested it manually.

Added beta evaluation.

Testing JavaScript using eslint


core/modules/ckeditor/js/ckeditor.admin.js
  108:10  error  'openGroupNameDialog' is not defined  no-undef

core/modules/ckeditor/js/views/ControllerView.js
  354:1  error  Unexpected blank line at end of file  eol-last

✖ 2 problems

eslint test failed

Darn, I really need to write a drupallint script to run the various coding standards linters to prevent this.

Manually tested again.

Committed b61edd7 and pushed to 8.0.x. Thanks!