Problem/Motivation

There are some parts of the CKEditor plugin code that aren't documented. Ensure that all classes and functions have at least minimal documentation.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork drupal-3248425

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lauriii created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)

Is this in the JS or PHP code?

Can you list a few examples?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Postponed (maintainer needs more info) » Active

Sorry, just realized that CKEditor plugin code is vague because we have plugins, both on the backend and the frontend. This issue was specifically for documenting our CKEditor plugins written in JavaScript.

Some examples:
https://git.drupalcode.org/project/ckeditor5/-/blob/1.0.x/js/ckeditor5_p...
https://git.drupalcode.org/project/ckeditor5/-/blob/1.0.x/js/ckeditor5_p...
https://git.drupalcode.org/project/ckeditor5/-/blob/1.0.x/js/ckeditor5_p...
https://git.drupalcode.org/project/ckeditor5/-/blob/1.0.x/js/ckeditor5_p...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Ensure that all classes and functions are documented » Ensure that all classes and functions in Drupal-specific CKEditor 5 plugins are documented

👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 9.3.x-dev
Component: Code » ckeditor5.module

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +stable blocker
Parent issue: » #3238333: Roadmap to CKEditor 5 stable in Drupal 9
Related issues: +#3248430: Improve Drupal.ckeditor5 documentation
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 9.4.x-dev » 10.0.x-dev

nod_ opened merge request !1970

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

What a leap forward! :D

Lots of nits (most of which already have suggestions ready to be applied, but … I can't apply them even though I have push access 😬), a few questions 🤓

yogeshmpawar made their first commit to this issue’s fork.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Still many function with missing documentation

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

I think i got most/all of them

nod_’s picture

nod_
Primary language French
Location Lille
commented

Thanks, fixed :)

nod_ credited marcvangend.

nod_’s picture

nod_
Primary language French
Location Lille
commented

adding credit

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I really wanted to RTBC this, but we need @lauriii to answer two questions on the MR.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3275237: Don't convert, instead use response.entity_type in DrupalImageUploadEditing

Converted the one @todo that was discovered while working on this MR into a new issue because it'd be out of scope to tackle it here: #3275237: Don't convert, instead use response.entity_type in DrupalImageUploadEditing.

That was the last thing to sort out! 🥳

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs reroll after #3245720: [drupalMedia] Support choosing a view mode for <drupal-media>.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Checked the merge, all good, thanks :)

bnjmnm made their first commit to this issue’s fork.

nod_’s picture

nod_
Primary language French
Location Lille
commented
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

Made it through every file!

Mostly nits in the MR. Setting to NW for visibility.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

Thanks, sorry for the test spam, needed to go through each suggestion at a time since some of them were not applying.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

That all looks good to me! 👍

Bumping priority given that this is a CKEditor 5 stable blocker.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Would have been slightly easier to review the @inheritDoc @inheritdoc and @internal -> @private changes in their own issue for a quick commit there and a smaller diff here.

Found a few nits but otherwise looks like a big improvement.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

Thanks for the review, think I adressed everything

  • catch committed ad44453 on 10.0.x 
    Issue #3248425 by nod_, yogeshmpawar, Wim Leers, lauriii, bnjmnm,...

  • catch committed 5b66668 on 9.4.x 
    Issue #3248425 by nod_, yogeshmpawar, Wim Leers, lauriii, bnjmnm,...

  • catch committed 8dedb39 on 9.3.x 
    Issue #3248425 by nod_, yogeshmpawar, Wim Leers, lauriii, bnjmnm,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.3.x-dev
Status: Needs review » Fixed

Thanks! Since this was just nits and already RTBC, went ahead and committed.

Then... I realised this changes a couple of method parameter names, so I probably should have waited for the test bot. Might as well wait and see if that was a bad idea or not since it's already in.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Should be safe, the generated JS didn't change even with the param name change. Strong suggestion it won't break :)

nod_’s picture

nod_
Primary language French
Location Lille
commented

Tests are green FYI

Status: Fixed » Closed (fixed)

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