ILIAS-eLearning
UI: Replace legacy modals in ilTestPlayerAbstractGUI
These changes are part of the Legacy-UI refactoring. This PR revises and brings together #7983 and #7984.
@matheuszych and I replaced the legacy ilModalGUI with the newer Kitchen Sink interruptive and roundtrip Modal. The control of the modal via DOM elements in JavaScript has been replaced by using signals. Class ilTestPlayerConfirmationModal and associated template files have been removed as they are no longer needed.
In addition, the commit cf00c9d was reverted to fix lObject::read(): Object with obj_id: -1 (tst) not found!. The behavior described in the corresponding Mantis ticket has not reappeared here.
Sorry @lukas-heinrich ! Didn't want to do that: Neither the comment nor closing the PR!
Hi @kergomard !
Thank you very much for the feedback. I will implement the suggestions and update them in the PR.
I also tried to find out why the buttons in the bottom navigation bar are not working. This seems to be due to the browser placing them in the wrong place in the DOM. I was able to trace the behavior back to the fact that modals are rendered in the player's form, which themselves contain other forms. This leads to incorrect behavior because HTML forms are not allowed to have other HTML forms.
I have adjusted my relevant parts, but unfortunately I could not fix the issue completely. The modal .il-copg-mob-fullscreen is still in the form element and I have currently found no way to render it outside the form. I created a Mantis ticket for this specific modal: https://mantis.ilias.de/view.php?id=42364
Best regards, @lukas-heinrich
Hi @kergomard !
I renamed the variable and moved the modals out of the form as described.
With regard to this line of code, I can't tell which solution is the right one. I get the problem that $this->test_id is -1 when I delete an already answered question. After I click on the confirm button in the modal, the script exits with an error. Unfortunately, I can't tell how the program should behave correctly at this point. If desired, I can undo the revert and open a Mantis ticket for the issue.
I have also noticed that the pipeline has failed again. I don't really understand why, any hints are welcome.
Best, @lukas-heinrich
Thank you very much @lukas-heinrich !
Now it does work. There is one last thing: The answer feedback modal has a funky background-color.
And: Yes, please revert the change and add an issue. I will look into that.
After these two changes, I will merge.
Best, @kergomard
Hi @kergomard !
Thank you for your support. I removed the changes and opened a ticket for the issue.
The formatting seems to be due to the fact that the feedback modal is an interruptive modal. If I interpret the documentation correctly, you can also use a roundtrip modal here and the modal is displayed in a better way (see screenshot).
I have this the change in a separate commit so that you can ignore it while merging if you do not want to modify the feedback modal.
Thank you very much @lukas-heinrich .
The idea with the roundtrip modal was the right one, as the text in the interruptive modal is always wrapped in an info-box and this box will have a yellow background. ...but there are more issues here:
- With the interruptive modal, the "continue" action, doesn't do what it should, i.e. it does not continue.
- Additionally, right now, if you set the question to be locked when navigating, this doesn't actually happen.
- What is more, you have some feedback info in the question, that part should have a colored background, and this doesn't happen in the interruptive modal either. For this to work we need to change some CSS and this we need to do through a clean PR for Timon, the maintainer, to review.
All this we are not going to fix in a spell and we also don't need to have this done by tomorrow. Could you maybe see with @thojou when you two would have the time, so we can have a look at this together in a call? Maybe @fhelfer and/or @matheuszych would like to join, too? I think this would be the best way to go forward. I would be available tomorrow after around 11ish or then next week.
Thanks again and best, @kergomard
Hi @lukas-heinrich When trying to figure out the whole functionality with @dsstrassner , I finally understood that the current behavior actually conforms to the specifications (the case I had found, that from where I stand is still not correct, none the less is as specified: If you set the locking to "Lock Answers After Moving to Next Question" and you have the feedback turned on, clicking away the modal will lead to a state that still lets you change your answer. You have to set locking to "Lock Answers After Feedback or Moving to Next Question" and only then the answer will really be locked, when clicking away the Modal). We sadly cannot get rid of the modal right now, as there are too many states that will be unclear to the user. We need to find a solution for this, but we are not going to find it now for ILIAS 10. Sorry, that I didn't grasp the full picture!
Could you please:
- See if you can find a solution for the display problems.
- Remove all camelCase names of properties you introduced.
- Remove the @throws.
- Rebase (it is trivial, as far as I can see as the only conflict should be in the test-class that is remove anyway).
Thanks and sorry again, @kergomard
Hi @kergomard !
Thank you for the feedback! I have visually revised the code and I found a solution for locking the answer while the dialog is already shown. It should now work as expected, I've tried tests with different combinations of with/without feedback and locking modes.
Best, @lukas-heinrich
Thank you very much @lukas-heinrich !
I merge this and will remove some of the comments myself to not torture you any longer with this. Merged and picked to trunk.
Best, @kergomard