Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/aut 3923/textreader image figure support #606

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

olga-kulish
Copy link
Contributor

@olga-kulish olga-kulish commented Nov 8, 2024

Related to https://oat-sa.atlassian.net/browse/AUT-3923

Changes

  1. mediaAlignment/helper
    In TextReader PCI, parent.contentModel && parent.contentModel === 'inlineStatic' didn't pass because parent.contentModel was undefined. Because widget.refresh didn't run, something wasn't updated, and image align reverted to Inline.
    It seemed fine to always call widget.refresh instead of active/sleep/active for other cases (A-block, ChoiceInteraction choice), not only for Prompt. Only for prompts of MathEntry/Likert PCI, there was now "QTI XML compilation error" when there was <figure> in the prompt (figure wasn't allowed before, for same reason as in TextReader PCI), but it was solved by another change in Fix/aut 3923/textreader image figure support extension-tao-itemqti#2613
    This code was added in fix/AUT-2673/inline-figure-img #528, and refresh was added at the last moment.

  2. FigureStateActive
    Image Caption textarea in Authoring side panel: it was completely collapsed for newly added images. Add some min-height to it. Image Caption textarea appears after you wrap inline image.

Test

On unit05. Related PRs:

Copy link

github-actions bot commented Nov 8, 2024

Coverage Report

Totals Coverage
Statements: 76.88% ( 14349 / 18665 )
Methods: 80.23% ( 1814 / 2261 )
Lines: 81.39% ( 8590 / 10554 )
Branches: 67.44% ( 3945 / 5850 )

StandWithUkraine

Comment on lines 25 to 46
const searchRecurse = (parentElement, serial) => {
if (!parentElement) {
return null;
}
if (parentElement.serial === serial) {
return parentElement;
}
let found = null;
_.some(parentElement['elements'], childElement => {
if (childElement.serial === serial) {
found = parentElement;
} else if (childElement['elements']) {
found = searchRecurse(childElement, serial);
} else if (childElement['prompt']) {
found = searchRecurse(childElement.prompt.bdy, serial);
}
if (found) {
return true;
}
});
return found;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagging @marpesia to see if she remembers something about the problem this code was added to solve 2 years ago.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is working well on Authoring.
It rings a bell that it was done to be backwards compatible with previous items. This code was implemented at the end of 2022. Maybe it won't be an issue anymore.
I couldn't find any test exported from that time to validate my thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the same code on itemqti. I think it is to replace any old item with the Figure tag. oat-sa/extension-tao-itemqti@ba70db0

Regarding the refresh() part, I remember we had some issues updating the CKEditor and keeping the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I checked it again and found at least one difference: in A-block, when figure is added in the middle of text inside <p>, the content of <p> is split to mutliple lines ('<figure> is not allowed inside <p>? And Prompt or TextReader use <div> not <p>).
In my version, user wouldn't see that until he focuses text in A-block again, or reopens the item.

Screenshot_21

I'll restore old version, and add TextReader PCI check. Not sure how to check for A-block. As for contentModel, both A-block and TextReader have contentModel='itemBody'. And .qti-flow-container in GapMatch/Hottext doesn't have this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the A-block, I tested on oat-dev to see differences, and the behaviour is the same.
<Figure> is a block time, so it can't be inside a <p>.
I think it is ok.

@oatymart oatymart requested a review from marpesia November 11, 2024 10:14
@olga-kulish olga-kulish force-pushed the fix/AUT-3923/textreader-image-figure-support branch from 78326c9 to 3d48491 Compare November 12, 2024 09:03
Copy link

Version

Target Version 3.8.1
Last version 3.8.0

There are 0 BREAKING CHANGE, 0 feature, 4 fixes

@olga-kulish olga-kulish self-assigned this Nov 13, 2024
@olga-kulish olga-kulish merged commit 6670a87 into develop Nov 13, 2024
3 checks passed
@olga-kulish olga-kulish deleted the fix/AUT-3923/textreader-image-figure-support branch November 13, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants