Expandable Standard Panel and Standard Listing Panel

[tfamula]

This PR introduces an extension to the Standard Panel and Standard Listing Panel to make them expandable, as it is need for https://docu.ilias.de/goto_docu_wiki_wpage_7416_1357.html. The PR includes the interface changes and also the implementation part of it.

The following things are missing and will be added if interface ~~and implementation are~~ is accepted:

• [x] CSS adjustments for the Bulky Buttons in the Panel header (including the requirement in the FR: "[...] the clickable area should be as large as possible so that the user can also click next to the panel title to open or close it.")
• [x] Adding / fixing unit tests

~~Please also consider, that the Javascript part was tested on ILIAS 9, since Javascript is not working yet on current trunk.~~

[Amstutz]

Hi @tfamula

Note, this is just a review of the public interface and the examples, NOT the full implementation. If the public interface is accepted by the JF, a further review will follow. Further note, that Unit tests would also be needed to complete the Review (including the JS part). Finally, note that we encourage to write the unit tests along with the implementation to find potential design issues early in the process.

Some questions:

• [x] Examples: You added things not strictly having a relation to the panels in the examples (such as pagination). Maybe simplify them, but extend the examples on the panels usage somewhat further (such as the async loading or action part).
• [x] Expandable Usage: Why aren't all panels expandable?
• [x] Interface Type: Why does the interface extend "Component"?

Change or indicate why not:

• [x] Interface Methods: Remove all except "with Expandable" on the public interface. Clients do not need to know those things. Most of it is internal stuff used for rendering.

Please Change:

• [x] Standard Panel Description: Please adapt the description of the Standard Panel
• [x] Interface action parameter type: Do not pass a string as expand_action type. Use URI and Signal, maybe also null instead.

Panel is a component with a weak semantic and a rather broad set of possible usage scenarios. We expect that the support of the panel will be discontinued at some point in the future and ask, whoever is reading this, to use components with stronger semantics, such as presentation table or entities, whenever possible.

@Amstutz, @thibsy, @klees

[tfamula]

Hi UI Coordinators!

Thanks for your initial review! I see the point about units tests. I would like to have at least an OK for the public interface by you and the JF, before I start working on the tests. Thanks also for the reminder regarding the JS part.

• [x] Examples: You added things not strictly having a relation to the panels in the examples (such as pagination). Maybe simplify them, but extend the examples on the panels usage somewhat further (such as the async loading or action part).

I will try to make a suitable example for the KS docu regarding the action part. Personally, I would like to keep the View Control in this example to visualize that the View Controls will be hidden as soon as the Panel is collapsed (I have added this point to the description, it was missing).

• [x] Expandable Usage: Why aren't all panels expandable?

I have simply followed the decision of the UI Clinic: "We recommend extending Standard Listing Panel and Standard Panel with the expandable/collapsable behaviour."

• [x] Interface Type: Why does the interface extend "Component"?

I don't really have a reason for this. I have seen that interfaces like HasViewControls and JavaScriptBindable also do it, so I did it too because of consistency and just "to be on the safe side".

• [x] Interface Methods: Remove all except "with Expandable" on the public interface. Clients do not need to know those things. Most of it is internal stuff used for rendering.

You are right. Done!

• [x] Standard Panel Description: Please adapt the description of the Standard Panel

Please check if I have understood this correctly. In my eyes, it's like a little duplication because I have written something about the expandable behaviour in the interaction part of the rules.

• [x] Interface action parameter type: Do not pass a string as expand_action type. Use URI and Signal, maybe also null instead.

Seems reasonable. Done!

@tfamula

[matthiaskunkel]

Jour Fixe, 29 APR 2024: We highly appreciate the extension of the Standard Panel and Standard Listing Panel and accept the PR for ILIAS 10 / trunk.

[Amstutz]

Hi @tfamula

I unassigned my self for the moment. Please re-assign me, as soon as the implementation is ready for a final review. Thx!

[tfamula]

Hi @Amstutz The implementation is now ready for final review. I made the adjustments for the CSS, added / fixed some unit tests (PHP and JS) and also made changes for the Renderers so that the JS part can handle URIs as well as Signals.

Regarding the examples: I tried to extend them by the action part. I made one example with URIs and one example with Signals. Because the URIs are performed asynchronously, I do not really know how to make this behaviour visual to the user in the first example. But of course, she can check the "Network"-part of her browser to see that the actions are executed. Additionally, I could not use the session here because the KS does not provide a solution for this AFAIK. In the second example, Modals are triggered when expanding / collapsing the Panel at the same time, which is way more noticeable for the user.

@tfamula

[tfamula]

Hi @thibsy thanks for the feedback! I have tried to work through the points as well as possible.

• Union types: Did I? Can you show me where exactly? Because I do not see that I used two different conventions for the spacing around the | separator. But you are right, that there are different conventions in general, so a ruling would be great!

• C\Panel\IsExpandable I: Sounds reasonable. I changed it.

• Accessibility: I can not confirm the problem with the accessibility. Focusing on the (visible) Bulky Button using the keyboards works like expected (because always get focus by default). While the focus is on the button, pressing Return or Space also results in collapsing/expanding the Panel. Maybe just try again after I changed a lot things now, especially removing the usages of Bootstrap. But before that, I personally also did not had any problems.

• Accessibility, pt.1: I used the Bulky Button here to meet the requirement of the FR: "[...] the clickable area should be as large as possible so that the user can also click next to the panel title to open or close it." In my understanding, this means, that the Glyph, the title of the Panel and also the area to the right should be clickable. Using only a Glyph would result in making only the Glyph clickable. That does not fulfil the requirement and also make the functionality less accessible IMO. Moreover, the actions will be removed from Glyphs, see here and also here. With using a Link you mean a Bulky Link, right? I checked this and this does also not seem suitable. An URI is expected as third parameter, which is not optional. And using only an URI as action would conflict with Timon's feedback before: "Interface action parameter type: Do not pass a string as expand_action type. Use URI and Signal, maybe also null instead."

• Accessibility, pt.2: Yes, I was using Bootstrap functionality here. But I changed the behaviour now to something similar like it is proposed for the Presentation Table. But we now have to live with the fact that there is no "pretty" toggle animation anymore, which was provided by Bootstrap. I guess there are alternatives which can be made by pure CSS. If someone is interested in this, she can do this in a next step after this PR is merged.

• Accessibility, pt.3: I tried to replace as many CSS classes as possible for the expand-state by using data attributes. I could also remove some JS code which became obsolete.

• Accessibility, pt.4: Hopefully done as you imagined it. But still Buttons are used and not Glyphs for the reasons I mentioned above.

A few insights of my own about accessibility: To be honest, I am not sure if the current solution is accessible enough regarding telling a non-sighted person that the Buttons do expand/collapse the area under them. Since the Buttons do not work like a "toggle" here, adding an "aria-expanded" attribute seems not suitable in my eyes. Maybe we need something descriptive for the Buttons which explains that it either expands or collapses something. But I do not know any specific solution. And this problem is also existent in other UI componets with such an expand-collapse-behaiour, like the Presentation Table, with the difference that no Buttons are used there.

• I\Panel\Renderer I: I created a trait for the code duplications. This does not apply to the actions rendering because it's not duplicate code IMO, it's just similar. I have to distungish between expand/collapse respectively URI/Signal. Or perhaps I have misunderstood something. I also extracted the HTML heading part into a new template file.

• I\Panel\Renderer II: Thanks for clarification. Done! But could you maybe explain this sentence in more detail: "However, it becomes important now since we also depend on this internally."?

• I\Panel\Renderer III: This was needed for the Bootstrap functionality and is removed now.

• I\Panel\Renderer IV: Done!

• Conflicts: Done!

@tfamula

~~PS: Tests are failing (Copyright). I will have a look at this later.~~

[tfamula]

@thibsy and I had a meeting in which we discussed the accessibility issues. We came to the conclusion that the solution with two Bulky Buttons is not suitable to fit the accessibility criteria. Instead, a solution with only one button (in combination with Glyphs), which is part of the Panel's template, has been developed now. This solves the problem with the loosing focus. Additionally, the needed aria-* attributes has been added.

[tfamula]

Hi @thibsy, is there any news on this one?

[tfamula]

Hello @thibsy, I can understand the change requests. At the same time, I am a bit frustrated that these requests have not been communicated earlier. Im my eyes, most of them could have been even reported in a former iteration step. I firmly assumed that this PR is as good as merged. Now, we find ourselves in an unfavourable situation. There is simply not enough time to work through your points until "Coding Completed". On the one hand, I had to wait nearly two months for the recent update here. On the other hand, I am on holiday from tomorrow until end of October. So we will not see the Expandable Panels in ILIAS 10, which is a great pity, especially in view of the fact that this PR already started at the beginning of April.

[thibsy]

Hi @tfamula,

We totally understand your frustration, this should not have taken so long to review, or we should have notified you about the delay or issues sooner.

We share your goal to include this feature with ILIAS 10, but we also think that the integrity and quality of our code is an important goal, which we try to compromise as little as possible. Would you commit to iterate on this and implement the requested changes after we merge this PR (and once you are back from vacation)?

@alex40724 maybe you can answer our question on his behalf, since according to the last comment he is already on vacation.

We apologise for the inconveniences.

Kind regards, @Amstutz, @klees and @thibsy