Problem/Motivation

Same as contextual.js & co.
(the js is confusing to read)

See related issues.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task neither a bug nor a new feature; solely refactoring.
Unfrozen changes Unfrozen because it doesn't change anything, it only does internal refactoring: it splits up existing JS across multiple files. No contrib/custom JS would ever want to override this.
Prioritized changes The main goal of this issue is unblocking #2182153: [Meta] Document Drupal JavaScript using JSDoc, which is a documentation issue, and hence has long-term DX consequences for front-end developers using D8.
Disruption No disruption.

Proposed resolution

Split up ckeditor.admin.js

Remaining tasks

  • Evaluate if allowed at this point in the beta release.

User interface changes

No.

API changes

No.

Comments

Wim Leers’s picture

Issue tags: +CKEditor in core
Wim Leers’s picture

Issue tags: +Novice, +js-novice
el7cosmos’s picture

Status: Active » Needs review
FileSize
89.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,291 pass(es). View
nod_’s picture

Status: Needs review » Needs work

There is an extra , in ckeditor.admin.js but more importantly there are references to registerButtonMove and a couple other functions in the split files. Those functions are local to the ckeditor.admin.js scope. They can't work from an other file. Those functions needs to be added to the Drupal.ckeditor object so they can be used elsewhere.

el7cosmos’s picture

Status: Needs work » Needs review
FileSize
108.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8-split-ckeditor-admin-js-2174589-5.patch. Unable to apply patch. See the log in the details link for more information. View
22.6 KB

Anything else i missed?

Status: Needs review » Needs work

The last submitted patch, 5: drupal8-split-ckeditor-admin-js-2174589-5.patch, failed testing.

setvik’s picture

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.

setvik’s picture

Status: Needs work » Needs review
FileSize
108.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,573 pass(es). View

Rerolled against HEAD. No other changes.

Wim Leers’s picture

Status: Needs review » Needs work

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!

Wim Leers’s picture

Issue tags: +DrupalCamp Ghent 2014, +sprint
ikeigenwijs’s picture

Assigned: Unassigned » ikeigenwijs
ikeigenwijs’s picture

Assigned: ikeigenwijs » Unassigned
Status: Needs work » Needs review
FileSize
100.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,982 pass(es). View
Wim Leers’s picture

Status: Needs review » Needs work

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! :)

ikeigenwijs’s picture

Assigned: Unassigned » ikeigenwijs
ikeigenwijs’s picture

Assigned: ikeigenwijs » Unassigned
Status: Needs work » Needs review
FileSize
116.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,980 pass(es). View
18.75 KB

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

ikeigenwijs’s picture

FileSize
116.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,986 pass(es). View
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
110.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,394 pass(es). View
94.68 KB

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.
YesCT’s picture

FileSize
445 bytes

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

YesCT’s picture

Issue summary: View changes

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.

nod_’s picture

It's needed for us to be able to document our JS (JSDoc has problems when documenting all this stuff in one file for some reason, it's a JSDoc issue but it doesn't mean all this should be in a single file).

It's still the plan to do #2182153: [Meta] Document Drupal JavaScript using JSDoc It's just been a while since I could block another 12 hours to reroll that patch.

ikeigenwijs’s picture

thx Wim
So will the patch be submitted?

Wim Leers’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs manual testing

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.

Wim Leers’s picture

Issue summary: View changes

Added beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
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
Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
110.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,399 pass(es). View
1.32 KB

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

Manually tested again.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b61edd7 and pushed to 8.0.x. Thanks!

  • alexpott committed b61edd7 on 8.0.x
    Issue #2174589 by Wim Leers, ikeigenwijs, el7cosmos, setvik, YesCT:...
Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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