ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

TA: Vertical drag and drop order question design 33651

Open catenglaender opened this issue 2 years ago • 5 comments

Mantis issue: https://mantis.ilias.de/view.php?id=33651

Issue

Vertical and horizontal drag and drop elements in order questions look significantly different. It seems that style code from the dependency causes an unwanted inner block design.

2022-08-01 12_32_16-Window

Changes

Two challenges made this fix interesting:

  • The drag and drop dependency used (Nestable 2) brings a lot of unwanted style code with the default CSS classes it expects for various elements (e.g. "dd-handle")
  • there is a lot of PHP involved to assign the class names of the HTML elements rather than just writing them directly into the HTML templates.

To get the desired result I made the following changes:

  • The CSS Handle class had a getter and a setter, but wasn't actually filled into the JavaScript, so I made the placeholder tag work inside the JS config of the drag and drop as well
  • With a value for the CSS Handle Class actually rippling through, I could change the name of the CSS class from dd-handle to il-dd-handle which has no styling from the dependency associated with it. I left the names of other elements unchanged as their styling is not visual but important for the drag and drop functioning correctly.
  • to avoid a bug from the dependency with multiple classes for the handle div class, I nested the ilc_qanswer_Answer class in its own div div inside the handle div. This is how the horizontal ordering quiz is set up as well.
  • there was one font-size definition left over coming from the dependency that I had to override in the Test delos.less

More could be done

I wonder if this whole system should be refactored. Wouldn't it be much easier to simply write most of the classes plainly inside the the HTML templates without using placeholders? What is the common best practice here?

Outside of actual dynamic values like the ID, nested layer depth, and there is an ol/ul switch (which I am not sure we ever make use of) I do not quite see the need for it being so flexible, especially because that flexibility doesn't seem to be in use and is just filled with default values.

And maybe the horizontal and vertical drag and drop order questions could use a single dependency that supports both directions?

Unfortunately, both of these tasks will probably be quite laborious and might effect certain possible question configurations that I may not have an eye on. So that's why I choose this more minimal patch for now. If this spot is a priority or bigger changes are planned for this question type, we might want to revisit this.

catenglaender avatar Aug 10 '22 14:08 catenglaender

I am good with this one, thank you so much!

mbecker-databay avatar Aug 11 '22 07:08 mbecker-databay

Screenshots

Drag no nesting image

Drag with nesting image

catenglaender avatar Aug 16 '22 09:08 catenglaender

Hi @catenglaender

Thx a lot for this proposal. The CSS change looks fine for me. However, since we merge that into the productive 7 release, did somebody test this throughly? I could but, than I would need somewhat more time. With DOM changes I am always somewhat catious in TA due to not intended side effects on a JS level... If so, IMO this can be merged and picked to 7.

Concerning your questions:

Wouldn't it be much easier to simply write most of the classes plainly inside the the HTML templates without using placeholders? What is the common best practice here?

Completely agree. Not sure if there is a common practice in legacy html. IMO it is definitely bad practise to use place holders for classes used for styling. Designers are completely locked out this way. In UI Components we do no allow this. We should get rid of it here as well. However, I would not invest to much ressources in this atm, since we probably move lots of the rendering at some points to the UI Components anyway.

And maybe the horizontal and vertical drag and drop order questions could use a single dependency that supports both directions?

Sound right.

Thx for this.

Amstutz avatar Aug 18 '22 12:08 Amstutz

I have tested creation and the use of the changed ordering questions. So far, everything seems to work. (in Pools & Tests) But I am not happy with the styling in the nesting area of the creation process.

I will discuss this with @catenglaender after my vacation. So this one needs a little bit longer ;-)

dsstrassner avatar Aug 19 '22 13:08 dsstrassner

@dsstrassner Thx a lot for looking into this. I de-assigned myself ftm. Please don't hesitate to re-assign as soon as I can contribute something further here.

Amstutz avatar Aug 29 '22 14:08 Amstutz

Content Styles now load during editing of nesting order

The reason why the vertical drag and drop question appeared unstyled is because class assOrderingQuestionGUI did not load the content styles during the cmd editNesting. This has now been fixed.

Slight visual update

Nesting looks like this now with borders making the parent-child pairing even more clear and aiming a bit easier:

image

Some questions remain

Why are content styles loaded that selectively? Shouldn't the general assQuestionGUI always load the content styles instead of this being in the hands of a very specific command? When do we ever want to display questions without the content styles? Is that because of that print and result view?

catenglaender avatar Nov 16 '22 15:11 catenglaender

I have tested the new Style in @catenglaender branch. With the style is everything fine. But the Nesting Order is reset, if someone changes setting on the Edit Question Subtab (even no changes in Answer items, e.g. only the description is changed). I tried to test this behavior on test7, but there the nesting is not saved ... (Ping @nhaagen).

Also, I assigned @kergomard and @Amstutz - perhaps they can answer your question @catenglaender.

dsstrassner avatar Nov 20 '22 13:11 dsstrassner

Thanks for this PR from my side, too! I can actually not really answer your question without looking further into it (and this will have to wait for the moment). My guess would be that the test should centralize loading of the styles, but that for some historical reason it isn't. But as said: This is just educated guessing.

kergomard avatar Nov 21 '22 05:11 kergomard

At this point, I suspect one of the main reasons why the content styles are loaded selectively might be the print view. If we load the content styles with the global question rendering, the print view has them too, but that didn't seem to be desired. However, I think this potentially leads to bugs as it's easy to forget to bring style changes into the print view as well. Some questions (like kprim, or the vertical ordering) absolutely need styling to not look broken.

catenglaender avatar Nov 21 '22 08:11 catenglaender

Unfortunately I have no answer to the content style related question. @dsstrassner Maybe discuss in the squad on how to proceed here?

Amstutz avatar Dec 09 '22 13:12 Amstutz

Thanks @Amstutz. Yes, we discuss this pull request on the next TechSquad Meeting 2022/12/15.

dsstrassner avatar Dec 09 '22 14:12 dsstrassner

I have forgotten to comment, that I decided at the TechSquad Meeting (15.12.12), that @kergomard will merge this PR and cherry-pick accordingly to ILIAS 8 & Trunk.

dsstrassner avatar Dec 22 '22 08:12 dsstrassner

@catenglaender : Sorry this go stuck with me for so long! Could you please rebase and force push? I tried to do this through the webinterface, but apparently that doesn't work, sorry. I will then merge it quickly.

kergomard avatar Jan 12 '23 18:01 kergomard

Rebased PR on top of current release_7. Merge should be possible now.

catenglaender avatar Feb 01 '23 17:02 catenglaender

Thanks @catenglaender / happy merge @kergomard 😄

dsstrassner avatar Feb 01 '23 19:02 dsstrassner

Thank you very much @catenglaender ! Will merge and cherry-pick to 8 and trunk.

kergomard avatar Feb 02 '23 10:02 kergomard