UI/DataTable: Summary Rows

[nhaagen]

Sparked by https://mantis.ilias.de/view.php?id=45093, this adds SummaryRows to the DataTable.

[dsstrassner]

Hi @oliversamoila, I think you should discuss with @thibsy how to proceed in the future with this PR 🙈

But we need this!

[dsstrassner]

@thibsy Ich würde hier gerne eine bezahlte Priorisierung anbringen 💰 aus dem T&A Budget, wenn es was bringt.

[matthiaskunkel]

Als Produktmanager von ILIAS unterstütze ich diesen PR ausdrücklich. Bekommen wir damit auch das Problem gelöst, dass man bei der neuen Tabelle nicht mehr die Zahl der Eintrräge sieht?

[dsstrassner]

Soweit ich das verstanden habe, sollte das auch die Zahl der Spalten oder Linien zählen können. Falls ich das falsch verstanden habe, wäre das auch noch sinnvoll @thibsy & @oliversamoila 👯

[thibsy]

Hi @dsstrassner,

Das würde in der Tat helfen – vielen Dank für dein Angebot!

Bei dieser Gelegenheit möchte ich kurz hervorheben, dass viel Arbeit in die UI-Koordination fliesst die pro bono erfolgt. Dadurch, kombiniert mit den aktuellen Gegebenheiten im Verein, ist unsere Kapazität derzeit eingeschränkt. Es ist deswegen enorm wichtig, dass es Institutionen gibt wie die Universität Hohenheim, die Leibniz Universität Hannover und die Universität Bern, die nicht nur neue Features, sondern den gesamten Prozess mitfinanzieren und unterstützen.

Das bedeutet nicht, dass PRs erst berücksichtigt werden, sobald Gelder fliessen. Vielmehr wird dadurch Kapazität geschaffen, die damit allen Beiträgen zugutekommt.

Freundliche Grüsse
@thibsy (als UI-Koordinator)

[thibsy]

Hi @nhaagen,

Thanks a lot for your contribution to the UI framework!

To answer @matthiaskunkel's question first: yes and no, it will be technically possible to create a summary-row that gives you the information of how many entries are rendered up to some point. But the current proposal lacks a concept for how we display summaries that are not tied to any of the columns. Currently a summary is directly tied to a column and it will not be visible otherwise. This could lead to some implications if the mechanic is 'misused' for another purpose than summarising a column.

About the concrete changes, please answer the following questions:

• Row commonalities: could you explain why you kept the C\Table\DataRow and C\Table\SummaryRow strictly separated? Why not use a common interface for rows here?

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

• C\Table\DataRetrievalWithHeaderSummary: I believe this interface and its addition to the <thead> element will be used wrong most of the time. As in your example, I will argue that the semantics of WHATWG do not match: "The thead element represents the block of rows that consist of the column labels (headers) and any ancillary non-header cells [...]". I believe your summary is not ancillary because it, per definition, is a summary of the data contained inside the tbody element, thus it should be summarised there instead. Since you already introduced a mechanic for this, I suggest to simply drop this interface again and let consumers yield a summary row before the other ones, if this position really matters. If there is good reason to keep this, the intent and background of it should be well documented on the interfaces.

Please implement the following changes:

• DataRetrievalWithHeaderSummary::getHeaderSummary(): only if we really need to keep this interface: please provide an instance of the ILIAS\Data\Facrory as first argument, so consumers do not need to inject this themselves.
• tpl.datatable.html: only if we really keep this mechanic: please change the <th> to a <td> for the ancillary information.
• DataRowBuilder::buildSummaryRow(): you create an inline instance of the ILIAS\Data\Facrory there. Please use proper DI and inject it though all necessary layers starting from the bootstrap.
• Duplicate code: please get rid of the duplicated code inside I\Table\DataRow and I\Table\SummaryRow, either by creating a base class or using traits.
• I\Table\Renderer::fillCells(): please refactor this method and split the rendering aspects into separate functions for row commonalities and data- and summary-row specifics.
• Method signatures: please update the method signatures of your data retrievals (interface and example), there has been a change from ?array to mixed.
• C\Table\SummaryRow::getColumns(): you need to either import the appropriate Column class or use an FQDN there (PHPDoc).
• Unit tests: please provide at least one deliberate rendering test of the new summary row. Even better in different variations (table with multi-actions / single-actions / both).
• Rebase: please rebase this onto latest trunk to resolve conflicts.

Kind regards,
@thibsy (as UI coordinator)

[thibsy]

@nhaagen let us know if we (as in the ILIAS community) should take over from here.

[dsstrassner]

Dear @thibsy, we have left some time at CaT and I would kindly ask if @nhaagen and @klees could look further into this PR, as this function is important for me and the T&A as stated above.

Okay for you?

[thibsy]

@dsstrassner yes, of course!

[nhaagen]

Hi @thisby and all,

• Row commonalities: could you explain why you kept the C\Table\DataRow and C\Table\SummaryRow strictly separated? Why not use a common interface for rows here?

The common interface for rows is on the agenda for quite a long time now, and from my point of view
mainly the PresentationTable needs alignment to achieve this.
However, as this particular case shows, there are not really too many commonalities besides getColumns,
and I wonder if a component will really ask for "any" row, i.e. - will there be a consumer accepting a generic row?

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

• C\Table\DataRetrievalWithHeaderSummary: I believe this interface and its addition to the <thead> element will be used wrong most of the time. As in your example, I will argue that the semantics of WHATWG do not match: "The thead element represents the block of rows that consist of the column labels (headers) and any ancillary non-header cells [...]". I believe your summary is not ancillary because it, per definition, is a summary of the data contained inside the tbody element, thus it should be summarised there instead. Since you already introduced a mechanic for this, I suggest to simply drop this interface again and let consumers yield a summary row before the other ones, if this position really matters. If there is good reason to keep this, the intent and background of it should be well documented on the interfaces.

The HeaderSummmary is a special case to provide information about the whole data, not just the visible rows, and therefore should have a special place to optimize queries.
Also, adding it to the head will prevent the line from scrolling.

Please implement the following changes:

Thank you for your remarks, I will have a deeper look soon and provide an iteration.
Also thanks to @dsstrassner for funding that!

[thibsy]

Hi @nhaagen,

Thx for the feedback and for continuing your work.

• Row commonalities: I agree with you, a common interface would not be very beneficial at the moment. If the refactoring of the presentation table or future developments spawn a use-case where this is practical, we can still implement it then.
• C\Table\DataRetrievalWithHeaderSummary: I see the value of having a separate method to produce a summary row for the entire dataset. Not so much for query optimisation, but because we have a separated row that is a little different to the other summary rows, and, also because it provides the possibility to address this row explicitly during rendering. If we go this way, this should be made very clear (DataRetrievalWithDatasetSummary::getDatasetSummaryRow(): SummaryRow;). That said, I still believe your placement inside the <thead> element is semantically wrong. This information is not ancillary to the column headers, but rather ancillary to the rows (visible or not) of your table content (i.e. <tbody>). The argument about stickiness is a presentational one, which can be addressed differently, e.g. with above's separation.

I am looking forward to the next iteration.

Kind regards,
@thibsy (as UI coordinator)

[nhaagen]

• DataRetrievalWithHeaderSummary::getHeaderSummary(): only if we really need to keep this interface: please provide an instance of the ILIAS\Data\Facrory as first argument, so consumers do not need to inject this themselves.
• tpl.datatable.html: only if we really keep this mechanic: please change the <th> to a <td> for the ancillary information.
• DataRowBuilder::buildSummaryRow(): you create an inline instance of the ILIAS\Data\Facrory there. Please use proper DI and inject it though all necessary layers starting from the bootstrap.
• Duplicate code: please get rid of the duplicated code inside I\Table\DataRow and I\Table\SummaryRow, either by creating a base class or using traits.
• I\Table\Renderer::fillCells(): please refactor this method and split the rendering aspects into separate functions for row commonalities and data- and summary-row specifics.
• Method signatures: please update the method signatures of your data retrievals (interface and example), there has been a change from ?array to mixed.
• C\Table\SummaryRow::getColumns(): you need to either import the appropriate Column class or use an FQDN there (PHPDoc).
• Unit tests: please provide at least one deliberate rendering test of the new summary row. Even better in different variations (table with multi-actions / single-actions / both).
• Rebase: please rebase this onto latest trunk to resolve conflicts.

[nhaagen]

Hi @thibsy , I think this is good for the next round.