Skip to content

Conversation

@michaelsippel
Copy link

As described in #159 , .print_html() produced wrong html for title-rows.

This PR fixes this issue by introducing a boolean title flag in Cell::print_html / Row::print_html to select the appropriate tag (td or th).

title rows used improper <th><td> instead of <tr><th>
@michaelsippel michaelsippel changed the title Fix html-output for title rows Fix HTML output for title rows Jan 21, 2024
@pinkforest
Copy link
Collaborator

I would not like to break the API and the API would be confusing that adds thinking overhead when it could be automatic.

Feel like there should be internal state that tracks whether it is the first row and then use that as implementation detail ?

@michaelsippel
Copy link
Author

Thanks for the feedback @pinkforest !
You are right, I didn't really consider Cell::print_html() to be part of the API since usually you would call Table::print_html() instead of directly calling Row::print_html() or Cell::print_html() most of the time.
There, no additional thinking overhead would be added, since titles are added via Table::set_titles(), which sets the separate titles member of Table.
From there the information whether the cell is a title-cell or not is passed onto Row::print_html() and Cell::print_html() , where this information was not available before.

Deciding the cell type should not be based upon the order, since it is already given by the table as a separate row.

To eliminate the API-change in Row and Cell,
I prepared a new patch which adds cell_type to each cell indicating whether the cell is a title-cell (CellType::Head) or a data-cell, which is the default (CellType::Data).
Now, all print_html() function signatures remain intact and Cell simply uses its cell_type to decide between outputting <th> or <td>. The cell-type for all cells in the title-row is then automatically set by Table::set_titles().

- adds `CellType` for each `Cell` with corresponding getter/setter
- in `Cell::print_html()` choose `<th>` or `<td>` based on `cell_type`
- add `Row::set_cell_type()` to set type for all cells in a row
- always set title-row to `CellType::Head`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants