Wip800212 #5
Wip800212 #5
Conversation
There was a problem hiding this comment.
Thanks @BruceGoodGuy for working on this issue. It looks pretty much perfect, with very good documentation. I have just made a few minor comments in the code and would be nice to consider these bits.
- As this is a major change, it would be nice to update the version.php
- As we have removed Standalone version, it would be nice to update readme.md file and remove wherever it is referencing Standalone. And we no more use the Third-party plugin.
- Regarding the PHPDoc error, I think its safe to add the below step in workflow ci.yml file for Moodle PHPDoc Checker. So our workflow would pass.
continue-on-error: true # This step will show errors but will not fail.
Please let me know if any concerns.
Best regards,
Anu
amd/src/editor.js
Outdated
| appendChild(parent, child) { | ||
| parent.appendChild(child); | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks like we could get rid of this function appendChild, as it's just a single line of code and we could directly call the appendChild on the node wherever its needed.
| @@ -106,27 +106,23 @@ public function use_editor($elementid, array $options = null, $fpoptions = null) | |||
| foreach ($groups['style1'] as $plugin) { | |||
| $groupplugins[] = array('name' => $plugin, 'params' => array()); | |||
| } | |||
There was a problem hiding this comment.
We aren't using $groupplugins and $group. We can clean up some unnecessary code from line 87 to 108, though we might want to throw the exception when $options['supsub'] is invalid?
| saveHistory() { | ||
| const content = this.getCleanHTML(); | ||
| if (this.historyIndex === -1 || content !== this.history[this.historyIndex]) { | ||
| this.history.splice(this.historyIndex + 1); |
There was a problem hiding this comment.
Not sure if we need the below line of code?
this.history.splice(this.historyIndex + 1);
There was a problem hiding this comment.
Sorry @AnupamaSarjoshi :(,
I affair that I can't remove that line of code due to:
The line this.history.splice(this.historyIndex + 1); is important because it makes sure that when a new edit is made after using "Undo," the "Redo" options are cleared. Let’s break it down with an example:
Without splice:
Initial Edits:
The user types: edit1, edit2, edit3.
History: [edit1, edit2, edit3]
Index: 2 (pointing to edit3).
Undo Action:
The user undoes once.
History: [edit1, edit2, edit3]
Index: 1 (pointing to edit2).
New Edit:
The user types edit4.
History (without splice): [edit1, edit2, edit3, edit4]
This is incorrect because edit3 is no longer relevant but still exists in the history. The undo/redo behavior will become confusing.
With splice:
Initial Edits:
The user types: edit1, edit2, edit3.
History: [edit1, edit2, edit3]
Index: 2.
Undo Action:
The user undoes once.
History: [edit1, edit2, edit3]
Index: 1.
New Edit:
The user types edit4.
The line this.history.splice(this.historyIndex + 1); clears everything after edit2.
History (with splice): [edit1, edit2, edit4]
Now the history is correct and matches what the user expects.
Why Keep It?
Without splice, the history can keep old edits (edit3), which makes undo/redo confusing. By keeping splice, the history stays linear and predictable, like most users expect.
Thanks.
There was a problem hiding this comment.
Thanks @BruceGoodGuy, for explaining in detail. That's helpful :)
BruceGoodGuy
force-pushed
the
wip800212
branch
from
171e5a9 to
6be3414
Compare
|
Hi @AnupamaSarjoshi, |
|
Thanks @BruceGoodGuy, the changes look good and merged :) |
* Change SubSup editor to work with javascript and remove YUI version. * Update behat test. * Remove standalone superscript subscript editor. * Remove redundant data. * Update git ci.yml and readme.md --------- Co-authored-by: Khoa Nguyen Dang <[email protected]>
Hi @AnupamaSarjoshi,
CC @timhunt,
I’ve completed the update for the OUSupsub text editor. The new version is now built using pure JavaScript with no dependencies on libraries like YUI or RangyJS, while maintaining the behavior of the old editor. I’ve broken the changes into five small commits:
Commit 1: SubSup: New non-YUI version (#800212)
This contains the source code for the new version of the editor.
Commit 2: SubSup: Update Behat test (#800212)
The existing Behat tests no longer work due to the removal of YUI and RangyJS. I’ve updated the tests to align with the new changes. Although some steps were adjusted, the overall behavior remains unchanged.
Commit 3: SubSup: Remove redundant data (#800212)
This commit removes all YUI code and other obsolete files.
Commit 4: SubSup: Update Git ci.yml (#800212)
The ci.yml file was outdated, as Moodle's main branch has moved from ‘master’ to ‘main’. This commit corrects that.
Commit 5: SubSup: Fix Git action errors (#800212)
This commit addresses CI errors for the entire OU SupSub editor.
One issue remains:
In editor/ousupsub/lib.php:71, there’s an incomplete PhpDocs parameter list for the ousupsub_texteditor::use_editor function. This happens because the parent abstract method use_editor (in lib/editorlib.php) has an incorrect parameter type, which is outside this repository.
Could you please review these changes?
Thanks!