Posted by nod_ on
Problem/Motivation
Same as contextual.js & co.
(the js is confusing to read)
See related issues.
Beta phase evaluation
| 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: Use 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.
Files:
| Comment | File | Size | Author |
|---|---|---|---|
| #30 |  2174589-30.patch | 110.39 KB | Wim Leers |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,399 pass(es). [ View ] | |||
Comments
Comment #1
Wim Leers commentedComment #2
Wim Leers commentedComment #3
el7cosmos commentedComment #4
nod_ commentedThere is an extra
,in ckeditor.admin.js but more importantly there are references toregisterButtonMoveand 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 theDrupal.ckeditorobject so they can be used elsewhere.Comment #5
el7cosmos commentedAnything else i missed?
Comment #10
setvik commentedHi 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.
Comment #11
setvik commentedRerolled against HEAD. No other changes.
Comment #12
Wim Leers commentedI 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!
Comment #13
Wim Leers commentedComment #14
ikeigenwijs commentedComment #15
ikeigenwijs commentedComment #16
Wim Leers commentedregisterGroupMove()andopenGroupNameDialog()still need the same treatment asregisterButtonMove()already received: they need to be moved onto theDrupal.ckeditorobject. Then this should be good to go! :)Comment #17
ikeigenwijs commentedComment #18
ikeigenwijs commentedregisterGroupMove() and openGroupNameDialog()
changes added to patch
interdiff.txt between patch of comment 15 en 18
Comment #19
ikeigenwijs commentedComment #20
Wim Leers commentedWorks perfectly.
But:
Model.js, there was no empty line. So, added that.Comment #21
YesCT commentedHere is the result of git diff -w from 18 to 20.
Comment #22
YesCT commentedEvaluating 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.
Comment #23
nod_ commentedIt'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: Use JSDoc It's just been a while since I could block another 12 hours to reroll that patch.
Comment #24
ikeigenwijs commentedthx Wim
So will the patch be submitted?
Comment #25
Wim Leers commented#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.
Comment #26
alexpott commentedThe 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: Use JSDoc then ok - but test evidence please.
Comment #27
Wim Leers commentedI'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.
Comment #28
Wim Leers commentedAdded beta evaluation.
Comment #29
alexpott commentedTesting JavaScript using eslintcore/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
Comment #30
Wim Leers commentedDarn, I really need to write a
drupallintscript to run the various coding standards linters to prevent this.Manually tested again.
Comment #31
alexpott commentedCommitted b61edd7 and pushed to 8.0.x. Thanks!
Comment #33
Wim Leers commented