Skip to content

UI: Add Column Type List#9522

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

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

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 kitchen sink

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants