Skip to content

UI: Add Column Type List#9522

Closed
kergomard wants to merge 7 commits into
ILIAS-eLearning:trunkfrom
kergomard:11/ui/add_truncation_to_list_row
Closed

UI: Add Column Type List#9522
kergomard wants to merge 7 commits into
ILIAS-eLearning:trunkfrom
kergomard:11/ui/add_truncation_to_list_row

Conversation

@kergomard

Copy link
Copy Markdown
Contributor

Hi all

To implement Move Role-Permissions-Table to DataTable. It would be very helpful to have a column-type "Listing". This PR adds it.

...but there is more: There was also the wish to have the columns be truncated. I added some function to the interface to make this possible, although I must admit that I've a hard time to find the right structure and nomenclature. I did not implement much of it as I would kindly ask you for your opinion before investing the time to programming the whole thing:

  • I think it is a good idea for it to be enabled with one single and clear function call even though we actually need to have two things happening: The truncation needs to be enabled and we need to have a text to be presented, when the cell is truncated. We could also provide a default text, but I would have no clue what it should be and I don't think there even is a viable option. I thus added a single mutator withTruncation(\Closure $truncated_text_closure), but two getters isTruncationEnabled(): bool and getTruncatedTextClosure(): ?\Closure. As said: nomenclature is tentative. All Column-types up to this points get all their properties through the constructor. I don't think this is the right solution in this case.
  • When it comes to the implementation, right now I thinking along the lines of internally expanding Listing\Ordered and Listing\Unordered with a function withTruncation(string $truncated_text) without exposing it on the interface. The Closure passed to Table\Column\Listing::withTruncation() would then be called in the Table\Column\Listing::format() and would receive the value as parameter. More complex context could be accessed by leveraging the fact that the value comes from a Generator. I don't think this is going to be beautiful, but right now it is the best I could think of, without breaking the separation of concerns.

I'm very grateful for any feedback!

Thank you very much and best,
@kergomard

@yvseiler

Copy link
Copy Markdown
Contributor

Hi @kergomard

Thank you a lot for contributing to ILIAS. You will get some further feedback, nevertheless, I would like to point out that the HTML validator spits out a few errors if you want to take this into account already (Line 5506-5621):

  • The role attribute must not be used on a tr element which has a table ancestor with no role attribute, or with a role attribute whose value is table, grid, or treegrid.
  • The role attribute must not be used on a th element which has a table ancestor with no role attribute, or with a role attribute whose value is table, grid, or treegrid.
  • The role attribute must not be used on a td element which has a table ancestor with no role attribute, or with a role attribute whose value is table, grid, or treegrid,
  • Attribute aria-colindex not allowed on element th at this point.
  • Bad value Ascending for attribute aria-sort on element th.
  • Bad value 0 for attribute aria-colindex on element td: Zero is not a positive integer.

If I can support you with finding these errors, please contact me.

kindly
@yvseiler

@kergomard

Copy link
Copy Markdown
Contributor Author

Thank you very much for the feedback @yvseiler ! I will check this once we are clear on the interface, I'm not really sure this issue really is related to my changes, but I will figure it out, when I get there.

Thanks again and best,
@kergomard

@klees

klees commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

Hi everyone,

@kergomard and I quickly discussed this today. We agreed that we want to try to treat the "truncation"-issue as a strictly visual problem, i.e. not exposing any functionality for truncation to PHP developers for the moment.

General idea is that for a List Column the table would e.g. show the first three entries of the list and then a button saying "show all 10 entries" to expand the row to show the rest.

I'm sure @kergomard will want to elaborate at some point, this is just a quick marker to keep everyone updated on the progress here.

Kind regards!

@klees klees assigned kergomard and unassigned klees Jul 15, 2025
@klees

klees commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

@kergomard Please pass back to me and ping me for the next iteration. Thanks!

@kergomard kergomard force-pushed the 11/ui/add_truncation_to_list_row branch 3 times, most recently from 2c0e3a4 to fc02719 Compare August 11, 2025 08:18
@kergomard

kergomard commented Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Thank you very much @klees for the talk and feedback!

I now implemented a approach that is fully based on convention:

  • Lists in Tables are currently always reduced to 2 items + a "Show 4 more items"-shy-button.
  • This is true for the column types 'List' and 'LinkList'.
  • The changes are actually mostly in ILIAS\UI\Implementation\Listing. I have not surfaced them to the interface though, but I think it could be an option to do so. I would then stick to the "convention over configuration"-approach and not make the amount of items configurable.
  • The implementation is kept minimal on purpose, there e.g. is no styling. I right now believe this to be the right approach here, but clearly we could do a lot of moving around.
  • The implementation looks like this:
    Bildschirmaufzeichnung vom 2025-08-11 10-02-38.webm

Thanks again and best,
@kergomard

@kergomard kergomard assigned klees and unassigned kergomard Aug 11, 2025
@kergomard kergomard force-pushed the 11/ui/add_truncation_to_list_row branch 2 times, most recently from 2ed2ae8 to 05d0c05 Compare August 11, 2025 09:00
@dsstrassner dsstrassner assigned thibsy and oliversamoila and unassigned klees Dec 1, 2025
@BettyFromHH BettyFromHH added the css/html Pull requests that propose changes to CSS/SCSS or HTML files. label Jan 21, 2026
@kergomard kergomard force-pushed the 11/ui/add_truncation_to_list_row branch 2 times, most recently from 1bcca6d to 521a500 Compare February 19, 2026 13:28
@kergomard

kergomard commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Hi @thibsy

As discussed personally: Could you please review this on the budget we reserved with you for the test questions.

Thank you very much,
@kergomard

@thibsy thibsy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @kergomard,

Thanks a lot for your contribution to the UI framework!
Also big thanks for your patience…

I agree with the previous statement of Richard to treat the truncation as a purely visual issue.

About the concrete changes, please answer the following questions:

  • Display limit value: could you elaborate why you have chosen 2 as a value for the truncation limit? Is it just preference, or is there some more meaning to it? Both is fine, I'm just curious =).

Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:

  • Wording: I personally dislike "truncation" to describe the implemented behaviour (even though it is quite accurate), because I will always think about the database transaction, and that "all or nothing" is collapsed in this context. Could we switch to another terminology? My suggestions are collapsing or clipping.

Please implement the following changes:

  • Context rendering: please implement the truncation behaviour, which is specific to the use of listings inside table columns, using context rendering. This way we can treat the issue as purely visual and the functionality does not leak into our composable objects layer. Introduce a Listing\TableColumnListingContextRenderer renderer, which takes effect if the listing is used inside a listing column. If you need some concrete directions, or if I should take over, please contact me on Discord.
  • Client-side rendering: your solution currently does quite a lot of rendering on the client. We try to render as much as possible on the server, and I think in this case especially it will simplify things. Please try and render the HTML already exactly as it should look like (what list elements are visible and not, if there is a button or not). Then you should use CSS classes to hide/show additional entries and only toggle those inside JS, probably amongst some a11y attributes.
  • Accessibility: please make sure you are using the appropriate attributes on the button element and/or the list. I think we should at least use aria-expanded and aria-controls on the button.
  • Display limit: please introduce a new UI\Component\Listing\TruncationDisplayLimit interface with a getDisplayLimit() method, so we can expose this config to the system. Have a look at how we did this for e.g. the AsyncRefreshInterval.
  • Client-side translations: you should use AbstractComponentRenderer::toJS() for populating translations on the client. Then, due to our bad JS infrastructure, you need to retrieve values from il.Language on the client like done for the TreeSelect. There's also sprintf which you can use to achieve the same result as on the server.
  • JavaScript binding: once a component becomes a JavaScriptBindable, it must always bind provided JS code. You currently only bind this, if you register the truncation logic. IMO you can also simplify the ID-attribute rendering and always inject an ID. This allows us to add the id= part to the template, which is consistently done so.
  • Translations: you remove several translations that seem unrelated. Why?
  • JS code style: please run eslint on your JS files and fix other marked issues manually.

Kind regards,
@thibsy (as UI coordinator)

@kergomard kergomard force-pushed the 11/ui/add_truncation_to_list_row branch from d82f62c to fe0d1f2 Compare July 1, 2026 08:11
@kergomard kergomard force-pushed the 11/ui/add_truncation_to_list_row branch from fe0d1f2 to 24becf7 Compare July 1, 2026 08:45
@kergomard

Copy link
Copy Markdown
Contributor Author

Thank you very much @thibsy !

I added your changes to this PR. The code gets far cleaner like this!

I've just one things:

  • The tests are failing. Github only shows me "error" as the reason, locally I get the error "ComponentRendererFSLoaderTest::testGetRendererSuccessfullyExtra...is not instance of expected class 'MockObject_ComponentRenderer_415a5cfc'*. I tried to figure this out, but then gave up quickly as I will not have the time today, but would need to let it wait for three weeks now, ...so maybe you can figure this out quicker than me.

The rest lgtm.

Thank you very much again and best,
@kergomard

@oliversamoila oliversamoila left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello everyone,
I’d also like to say a big thank you for this PR. I’m happy to see that it seems to have been possible to continue working on this after all.

Many thanks for the changes that have already been made – several of the points of feedback I had once previously shared informally with @kergomard are now obsolete.

I have the following outstanding remarks:

  • The changes now include a display limit of 3. I think that’s reasonable for various use cases that come to my mind. I’m not entirely sure whether a consumer could essentially override this – but that’s actually quite interesting, out of curiosity.
    Could you please included this behaviour in the ‘Rules’ section?
  • We’ve actually just come up with a different solution for ‘Show more’/‘Show less’ in #11444, which doesn’t involve a separate button. Why don’t we use this here as well, so as not to solve the same thing in different ways?
  • Sorting the column based on cell content does not work. Conceptually, we should simply take the string into account here. We already do that in other places. (List items are not sorted within a cell. So this doesn't apply to numbered lists, but it also doesn't apply to unordered lists.) If you need to discuss this further, please let me know.

Thank you for any additions and clarifications.

Best regards,
@oliversamoila (as UI Coordinator)

@thibsy thibsy added php Pull requests that update Php code javascript Pull requests that update Javascript code translations Pull requests that propose changes to ILIAS language files. labels Jul 4, 2026
@thibsy

thibsy commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Hi @oliversamoila,

Let me answer your questions, since I kind of hijacked @kergomard's PR due to him being absent (already or pretty soon).

  • Display limit value: I wondered about the concrete value myself, but I am still waiting for Stephans response to that. However, I don't believe this is crucial information at all – we can still tweak this value later.
  • Display limit config: I initially thought we should provide a mechanic to configure this value, but ultimately decided against it to reduce overhead and another design decision one could potentially make. The value is therefore hardcoded into the php renderer and cannot be manipulated. To keep this value flexible, I would not put the concrete value on any kind of interface description. I will add a sentence about the expand/collapse functionality in general though.
  • Toggle functionality: the "show more/less" functionality of [FEATURE] UI: add Listing\Inline and Listing\Entity\Grid component. #11444 is only for text content. Since a list is a more complex HTML structure, CSS-only is not enough. I tried to address the functionality with as little JS as possible, but we still need some to toggle between the state of the individual list items.
  • Sortation: this actually raises an important concern – thx for bringing this to my attention. I need to take another look at how the data table implements sortation in general. Maybe we can work something trivial out for this PR, but I believe the most sustainable solution to this will require a central service. I think something like this also came up during the LUI project meetings. Maybe something for the short-term roadmap?

Hope this clarifies some things.

Kind regards,
@thibsy

@oliversamoila

Copy link
Copy Markdown
Contributor

Thank you for your feedback.

  • Regarding the limit value, we’re waiting for Stephan’s feedback. I’m fine with that. This applies specifically to the value 3.
  • Regarding the “show more/less” functionality, I understand that it isn’t transferable as it stands. However, I would strongly urge that we keep the number of different solutions to a minimum.
  • The faulty sorting is not something for a roadmap, but for the integration of this PR. We cannot introduce a new column type whose sorting is already faulty and then wait or expect that someone will implement the correct sorting function later – especially when we know better ahead of time. The sorting must work, as otherwise the use of this column type will essentially be prevented and migrations from LegacyUI will be prevented rather than encouraged. (The central service mentioned should be responsible for storage of values (Sorting, Pagination, Column Selection etc.), but not for the general function of sorting.

Best regards,
@oliversamoila (as UI Coordinator)

@oliversamoila

Copy link
Copy Markdown
Contributor

Just a short addendum: Please provide separate language variables for the singular and plural forms and use them:

  • Hide one item and Hide %s items
  • Show one item and Show %s items
  • Ein Element ausblenden and %s Elemente ausblenden
  • Ein Element anzeigen and %s Elemente anzeigen

Many thanks and best regards,
@oliversamoila

@thibsy

thibsy commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Hi @oliversamoila,

Thx for the feedback. Let me elaborate on some points:

  • Sortation: the new column type actually supports sorting, its just not implemented in the example (like in most other column examples). Sorting and applying view controls are delegated to the Table\DataRetrieval and is handled by each consumer individually. Which brings me to my next point,
  • Central service: the storage of values like above and the computation of such values are two distinct problems, for both of which I believe a dedicated and central service would beneficial. The UI framework should not implement behaviour like this. I still believe @mjansenDatabay already raised this concern, because this is handled inconsistently already. Its unclear if we should e.g. sort inside the DB or in PHP.
  • Translations: I would rather not take on that overhead, so I will drop the amount placeholder from the translations. Using the same vocabulary like in other places (i.e. the CSS-only solution) should keep this more consistent anyways. If the amount is really necessary to convey information, we can revisit this later. This also brings me to another point,
  • Language component: I strongly believe the language component needs some attention. Decisions like when to use singular vs. plural should not be handled by the consumers. This should be decoupled from the business logic and treated inside the language component. There should also be an explicit interface to provide placeholder values like above, so the language service can choose the right form, gender, etc. according to the users settings. @katringross / @kevenClausenKPG I am happy to walk through some ideas with you.

Kind regards,
@thibsy

@thibsy

thibsy commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

I continue this PR as #11728 on behalf of @kergomard during his absence.

@thibsy thibsy closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

css/html Pull requests that propose changes to CSS/SCSS or HTML files. improvement javascript Pull requests that update Javascript code kitchen sink php Pull requests that update Php code translations Pull requests that propose changes to ILIAS language files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants