DateTime and Duration Inputs for Filter (follow-up of #6444)

[tfamula]

Follow-up of https://github.com/ILIAS-eLearning/ILIAS/pull/6444

[tfamula]

Former text:

Hello everybody!

This PR adds the recently revised DateTime and Duration Inputs for the Filter.

Although the general implementation seems to be fine, there are some issues I could not solve at the moment:

• Layout in Popover content: The Duration Input is rendered as a "complex" Filter Input in a Popover (see screenshot). This does not look that bad, but it could still use some polish. Maybe some UI experts could help here with the HTML/CSS. (Side note: Please ignore the wrong placement of the Popover. This is a general problem of the Popovers within the Filter – and maybe some other UI components – in the current trunk and should be tackled separately.)
• Method getUpdateOnLoadCode: This method is primarily used for "complex" Filter Inputs to synchronize the changes within the Popover with the Filter Field, which triggers the Popover. Unfortunately, it turned out to be difficult to make this method work for the new Inputs, especially for the Duration Input, which technically consists of two DateTime Inputs, what makes it more difficult. I've run out of ideas and probably will need some help here. But it's important to mention here, that this will not obstruct the functionality of the Inputs within the Filter. The problem is limited to usability and could theoretically be tackled as a separate issue.

@tfamula

[tfamula]

Hi @tfamula

Thanks a lot for your contribution to the UI components!

I apologise for the delay here, normally we try to get a first response out within about 10 days, but unfortunately we had to prioritise some other things for the release of ILIAS 9.

About the concrete changes, please answer the following questions:

• [x] FilterContextRenderer: Could you explain why we need wrapInPopoverContext() in addition to wrapInFilterContext()? The outcome seems to be similar-ish.

This new method is needed due to the technical characteristic of a Duration Input. During the rendering process, it is first rendered as a Group Input and after that the two Date Time Inputs are rendered. The Group Input represents the Filter Input and must be used by wrapInFilterContext() to give it the special look (Label on the left and Remove-Glyph on the right). But the two Date Time Inputs within the Popover shall not have this look. That's why I need this extra method (wrapInPopoverContext()) with the extra template. And that's also the reason why I use withPopoverView() – to distinguish between a Date Time Input which is either used individually in the Filter or in the context of a Duration Input. To understand better what I mean, you can search for the two usages of withPopoverView() and change one or both parameters from true to false (or remove the mutator).

Please implement the following changes:

• [ ] FilterInput: This class is only used to communicate information between rendering steps. However, the composable DTOs should not carry such information, so I suggest to implement a context-renderer for the DateTime input and get rid of this class again. If you have questions about this feel free to contact me on Discord.

The reason why I need this class is half-answered in the point above. I simply need an approach how to distinguish between individually rendered Date Time Inputs and those within a Duration Input. To work with mutators here sounds legit for me. Therefore I use the methods withPopoverView() and isInPopoverView(). I do not know a different technique to implement this. A context-renderer for the Date Time Input cannot help me in this case IMO because I need the information in the FilterContextRenderer (which is already a context renderer like the name suggests). By the way, there already exists an Interface called FilterInput to identify "complex" Inputs for the Filter, so I do not really understand why the class must vanish.

• [ ] Unit tests: Please add some unit tests, especially for the newly added tpl.context_filter_popover.html template (if its necessary, see question above), making sure your changes keep working in the future.

I tried to do this, but I failed. The problem is that the template is not testable because it is nested in a Popover and the content of the Popover is not reachable by the unit tests. I have tried to find maybe an example of a unit test which have a solution for that (by searching for "il-standard-popover-content"), but unfortunately there are none.

• [ ] SCSS: It's fine if something does not look completely polished. However, please improve the look of the popovers, so they do not look broken. The placement is a different issue, as you point out. See SCSS Guidelines here: https://github.com/ILIAS-eLearning/ILIAS/blob/trunk/templates/Guidelines_SCSS-Coding.md

This is not meant to sound like an excuse, but I do not know at which point this will look "good". That's why I asked if some UI experts can take care of the matter. The only thing that came to my mind was to add some margin for the Popover content and with this, the Popover does not look too bad. It also seems to work with small screens. Please check if I kept the SCSS guidelines correctly and if this change is already sufficient.

• [x] Conflicts: Please rebase these changes onto the latest trunk. This will most likely be a bit more complicated than usual due to the late changes of the component revision.

Done by opeing this new PR

Kind regards, @Amstutz, @klees and @thibsy

[tfamula]

Info: @thibsy and me had a meeting end of November. We talked especially about the context-renderer for the DateTime input, which should be introduced. The ball is in my court and I will implement it ASAP.

[tfamula]

Hi @thibsy!

I was finally able to tackle your feedback. As discussed in our meeting at the end of November, I introduced a new Context Renderer for DateTime Inputs, which are rendered within a Duration Input in Filters (see 194eba654b563f44a7e6f79b3ed23e2e5de6718c).

While working on it, I (again) stumbled over the inconsistency regarding the rendering of Inputs in Forms and Filters. The FilterContextRenderer is mostly a duplicate of the Renderer for Forms and additionally contains some extra Filter rendering stuff. But the classes to be maintained separately and that's the problem. In most cases, when a bug is reported for Forms/Inputs, it is only fixed in the "normal" Renderer and not in the FilterContextRenderer, although it is sometimes also relevant there. I personally do not feel responsible to "check the code from time to time" to straighten these differences. That's why I refactored these classes and also introduced a new AbstractRenderer, which ensures consistency for all stuff, which is the same in Forms and Filters. Please have a look at 46cd1d724e9b639c45a2ba8534c90972d207e90d. I know that this is a different topic. If you do not want to handle it in this PR, I can revert the last commit for now. Once this PR is merged, I would create a new PR which includes the refactoring stuff.

Happy holidays! Thomas

PS: I will have a look at the failing tests!

[tfamula]

Hey @thibsy, I just wanted to let you know that I will be working on this, but not very soon because I have other priorities at the moment. Thank you for your patience!

@tfamula

[tfamula]

Hi @thibsy I finally got around to continue, thanks for your patience! First of all, please note that I implemented the desired changes based on the ILIAS 9 branch, because the trunk is currently not on a level where the Filter can be tested properly, e.g. due to not found and executed Javascript files (see https://github.com/ILIAS-eLearning/ILIAS/pull/7019). When I was finished with my changes, I picked them to trunk and fixed some conflicts. In general, there are not many differences between release 9 and trunk regarding the Inputs, so everything should work as soon as the trunk is reasonably stable.

• [x] A11y: I have noticed that complex FilterInputs are not accessible, or at least I couldn't figure out how to reach the popover content using only the keyboard. Since we need to rethink this component (popover) anyways, I merely added this here for documentation. But if you have an idea how we can solve this, feel free to let us know :).

The cause of the problem are the not executed Javascript files which I mentioned above. The accessibility of complex FilterInputs is handled by the filter.js file (you can search for "Accessibility for complex Input Fields" within this file). I check this as "done", because it should work again as soon as the general problem in the trunk is solved. On ILIAS 9, it works. Complex FilterInputs can be reached with the keyboard by clicking Return or Space.

• [x] AbstractRenderer I: IMO we don't need an abstract base class for all of our input renderers. Instead of having a contract between concrete implementations and our base class, I think it would make more sense that more concrete renderers just extend a less concrete one and override its functionalities on-demand. I therefore suggest to make the other renderers extend from Renderer and get rid of the AbstractRenderer again.

I understand this point and implemented it like that! I had to also do some small changes in the Renderer, but they are actually trivial in my eyes.

• [x] @inheritDoc: you have implemented some new methods featuring this PHPDoc annotation. We have decided that this annotation can be dropped in the future, however, this is not yet enforced by any rule. So this point is optional for you.

Good to know! I dropped the annotation for my new methods.

• [x] AbstractRenderer II: it definitely makes sense having a base class for all of our input fields. However, I think the hierarchy is not entirely correct here: the DateTimeFilterContextRenderer should extend the FilterContextRenderer because they share commonalities in a sense that they do not implement a disabled state.

Done!

• [x] CSS: Thx for adapting to the new naming convention. Please use the templates/default/050-layout/basics/_layout_spacing-variables.scss for margins, here probably $il-margin-xxlarge-vertical and $il-margin-large-horizontal. We currently do not use em for margins.

Done!

• [x] Bug I: undefined will be set as string when updating the durations date-time inputs in the filter, see screenshot below.

Done! The "Proxy Field" is filled with the values of the two DateTime Inputs, which are supplied by the val() method.

• [x] Bug II: this may be caused by the previous bug: when submitting the filter, the duration input will not submit any values.

What I can say for sure is that this is not caused by Bug I. I strongly assume that this is/was a "trunk problem", because in my case on ILIAS 9, the values are submitted correctly.

• [x] Conflicts: please rebase these changes onto the latest trunk again to resolve the conflicts.

Done!

@tfamula

[tfamula]

Hi @thibsy! I have worked through the open issues. Let me explain some of them a bit more.

• Inconsistent values

At first, I had difficulties understanding what I had to do here and I am still not totally sure if I did it correctly. My first approach was to use Filter::getData(), because it returns exactly what we need here. But this does only work when a request is sent and this is only the case for some commands (apply, toggle on, toggle of). For the other commands in the Filter and also when the activated Filter is retrieved without any command, an error will come up because there is no request. So I had to copy the needed transformations to ilUIFilterService and apply them in case that an Input is a DateTime or Duration. Personally, I do not know how to improve this, but it would be desirable if the handling of the values is not dependent on ilUIFilterService. Please also note that the Result for the transformations is always Ok because there is no error handling in Filters like in Forms. Also because of this, not all transformations of DateTime and Duration in Forms are used here.

• Context rendering tests

I hope I have understood you correctly and created some rendering tests for Inputs which are rendered in the Filter context. I also added one (missing) rendering test to DateTimeInputTest.

• ID-handling

Thanks for the explanation, I didn't have that on my radar. For ColorPicker, the ID was indeed missing, right? For MultiSelect, you are right and I reverted the old state. For Duration, the old state would be still wrong. When executing Renderer::wrapInFormContext(), an ID ($first_input_id) was incorrectly passed. This led to the fact that the general label of the Duration Input pointed to the first DateTime Input, which has its own label. I identified this issue with the help of the rendering tests and of course adjusted them accordingly.

@tfamula

[tfamula]

Hi @thibsy,

it's done! But suddenly, the CS Fixer complains about the coding style. I could not find out what's wrong with it and for some reason, executing locally the command vendor/composer/vendor/bin/php-cs-fixer fix --stop-on-violation --using-cache=no --diff --config=./scripts/PHP-CS-Fixer/code-format.php_cs does not find any violations. Could you please have a look and fix this? If everything is fine afterwards, please merge.

@tfamula

[tfamula]

Hi @thibsy, thanks for the PR! Now, all checks have passed :tada:

[thibsy]

Thx @tfamula for incorporating the changes! I will take care of this PR tomorrow.

[thibsy]

@tfamula FYI: we will take this to JF to announce the minimal interface change, due to which the DateTime and Duration inputs become FilterInputs.

[matthiaskunkel]

Jour Fixe, 08 JUL 2024: Suggested change is highly appreciated and accepted for trunk.