ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

UI: Introduce Is Nullable to Certain Column Types

Open kergomard opened this issue 1 year ago • 5 comments

Hi everybody

This PR is first to start a discussion. I would also implement this afterwards, if we think some implementation is needed, but I would then ask you to be a little bit patient with me (this is outside any contract).

While working on the table to show logs in the test I had the requirement to add a column with optional content (in my case a link). This is currently not possible. Right now it is possible to render the link and then insert it as a text column, but I don't believe this to be the right solution either.

This PR thus proposes to add a function Column::withIsNullable(bool $nullable): void to certain column types, but not all.

Open questions and explanations:

  • I did not add withIsNullable to all column-types, because I believe that certain types, e.g. Status or Boolean profit from being clear: In these cases the options are clear and should always be defined. Other types contain primarily text (Textand EMail) and here I think it is better to force the use of empty strings for empty values instead of offering two different options to achieve the same thing, although this argument is a little controversial, as there still is a difference between the empty value and a non-existent value. Number in my eyes is different and a clear case of the latter, as we have a clear and visual difference between the neutral value, ie. 0, and a non-existent value, ie. null.
  • I think the TimeSpan is another interesting case, where my current thinking is that nullable should mean that either the left or the right or both values can be null. I would argue that this avoids the need to introduce more of these "configuration functions" as all this scenarios are very well possible (and I would argue will come up).
  • Finally sorting: I would suggest to add empty values to the end of the list, as I this feels more logical to me (fuzzy argument, I know). The argument for putting it first: Our main database sorts them in first by default (I know: changeable). I have no clear-cut opinion on this.

If I get it right the functions on the interfaces of the table come with little in the way of doc-blocks and I think this should be self explanatory, too, but I could add some information to the Column\Factory if desired.

Thank you very much and best, @kergomard

kergomard avatar Apr 26 '24 13:04 kergomard

This is now partially superseded by https://github.com/ILIAS-eLearning/ILIAS/pull/7566 . There are more column-types that might profit from some kind of "nullable" state (see above), but I don't have much of a horse in the race anymore. Depending on your decision, I'm still willing to further work on this.

kergomard avatar Jun 10 '24 12:06 kergomard

@kergomard I have assigned this back to you to get this off my list, feel free to ping me and assign back if there is a next iteration.

klees avatar Jun 11 '24 07:06 klees

Thank you very much for the Feedback @klees, @Amstutz , and @thibsy !

I realized something that I only figured out by a mistake: I don't believe anymore, that this is needed and I'm sorry to have caused you work!

There already is a mechanism for this: You just don't specify the corresponding key in the array passed to the DataRowBuilder. It just didn't cross my mind that this was an option. Am I missing something?

If this is the way to go, I would provide a PR to expand the examples correspondingly. If I'm wrong, and we would prefer to have this more verbose approach, I can go on working on this PR.

Sorry again and best, @kergomard

kergomard avatar Aug 06 '24 15:08 kergomard

I'm currently not sure if we support this deliberately or just by accident. If there is no pointer to that functionality in the examples or descriptions, I would guess this is by accident. I will look into this some time soon and try to give a more informed opinion.

Kind regards!

klees avatar Aug 06 '24 15:08 klees

Hello everyone, I stumbled across this and the related PR in https://mantis.ilias.de/view.php?id=39326 again. As a non-expert on the implementation details, can you give me some info on when we can expect a next step here? I'm just a little concerned that it's assumed that it's the other one's turn ;) Thank you very much for your work on this topic.

oliversamoila avatar May 16 '25 13:05 oliversamoila