fix/AUT-2673/inline-figure-img #528
Conversation
Coverage Report
|
marpesia
changed the title
src/figure/FigureStateActive.js
Outdated
| const $editableContainer = widget.$original.parents('[data-qti-class="_container"]'); | ||
| htmlEditor.buildEditor($editableContainer, {}); | ||
| _.defer(() => { | ||
| htmlEditor.destroyEditor($editableContainer); | ||
| }); |
There was a problem hiding this comment.
question: Why building then destroying the editor immediately?
There was a problem hiding this comment.
The CKEditor reads the Figure HTML tag, and if it is inside a block, it moves out of it.
To achieve this effect, I must rebuild the editor, but I don't want to have it active, so I destroy it afterwards. Otherwise, I will have an error in the XML structure.
src/figure/FigureStateActive.js
Outdated
| @@ -125,7 +127,7 @@ const initForm = ({ widget, formElement, formTpl, mediaEditor, togglePlaceholder | |||
| ); | |||
| }; | |||
|
|
|||
| export default function (stateFactory, ActiveState, formTpl, formElement, inlineHelper) { | |||
| export default function (stateFactory, ActiveState, formTpl, formElement, inlineHelper, htmlEditor) { | |||
There was a problem hiding this comment.
quibble: The signatures becomes heavier and heavier. Maybe it is the right time to think about simplifying it. If the API is not used in too many places, it could be simplified with a config object, allowing to addition of more entries without breaking the signature.
| if (className) { | ||
| widget.element.attr('class', className); | ||
| } else { | ||
| widget.element.removeAttr('class'); | ||
| } | ||
|
|
||
|
|
||
| if ((prevClassName || className.length) && prevClassName !== className) { |
There was a problem hiding this comment.
question: Why test the value only for one variable and the length only for the other variable? Both are string, so it could be tested by value for both...
suggestion:
| if ((prevClassName || className.length) && prevClassName !== className) { | |
| if ((prevClassName || className) && prevClassName !== className) { |
src/figure/FigureStateActive.js
Outdated
| * @param {Object} config.htmlEditor | ||
| * @returns | ||
| */ | ||
| export default function (config) { |
There was a problem hiding this comment.
suggestion: It could also have been turned into a restructured declaration as mentioned in the coding guide
| export default function (config) { | |
| export default function ({ stateFactory, ActiveState, formTpl, formElement, inlineHelper, htmlEditor }) { |
If some parameters are mandatory and need to be given anyway, it could also be split into groups:
| export default function (config) { | |
| export default function (stateFactory, ActiveState, { formTpl, formElement, inlineHelper, htmlEditor }) { |
There was a problem hiding this comment.
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- New code is not subject to concurrency issues (if applicable)
- Feature is working correctly on playground (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
There was a problem hiding this comment.
Great job! 🎉🎉🎉
- New code is covered by tests (if applicable)
- Tests are running successfully (old and new ones) on my local machine (if applicable)
- New code is respecting code style rules
- New code is respecting best practices
- Pull request's target is not
master - Feature is working correctly on kitchen env (if applicable)
- Acceptance criteria are respected
- Pull request title and description are meaningful
Version
There are 0 BREAKING CHANGE, 0 feature, 12 fixes |
Related to: AUT-2673
Parent Extension
Changes: