UI: Add Column Type List#9522
Conversation
|
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):
If I can support you with finding these errors, please contact me. kindly |
|
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, |
|
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! |
|
@kergomard Please pass back to me and ping me for the next iteration. Thanks! |
2c0e3a4 to
fc02719
Compare
|
Thank you very much @klees for the talk and feedback! I now implemented a approach that is fully based on convention:
Thanks again and best, |
2ed2ae8 to
05d0c05
Compare
1bcca6d to
521a500
Compare
|
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, |
There was a problem hiding this comment.
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\TableColumnListingContextRendererrenderer, 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-expandedandaria-controlson the button. - Display limit: please introduce a new
UI\Component\Listing\TruncationDisplayLimitinterface with agetDisplayLimit()method, so we can expose this config to the system. Have a look at how we did this for e.g. theAsyncRefreshInterval. - 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 fromil.Languageon the client like done for the TreeSelect. There's alsosprintfwhich 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 theid=part to the template, which is consistently done so. - Translations: you remove several translations that seem unrelated. Why?
- JS code style: please run
eslinton your JS files and fix other marked issues manually.
Kind regards,
@thibsy (as UI coordinator)
* Implement listing column using context rendering * Improve a11y of truncation, rename it too * Update JS and avoid client-side rendering
d82f62c to
fe0d1f2
Compare
fe0d1f2 to
24becf7
Compare
|
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 rest lgtm. Thank you very much again and best, |
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:
withTruncation(\Closure $truncated_text_closure), but two gettersisTruncationEnabled(): boolandgetTruncatedTextClosure(): ?\Closure. As said: nomenclature is tentative. AllColumn-types up to this points get all their properties through the constructor. I don't think this is the right solution in this case.Listing\OrderedandListing\Unorderedwith a functionwithTruncation(string $truncated_text)without exposing it on the interface. TheClosurepassed toTable\Column\Listing::withTruncation()would then be called in theTable\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 aGenerator. 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