Ensure that header component HTML tags pass HTML validation #235
Conversation
|
Can you paste in what you saw that indicated something was fixed? Either the output of a tiny reprex, or a screenshot of what you looked at that indicated things were fixed? |
|
When reviewing this PR, I see 3 changed snapshots. Without guidance in the PR description, it's not clear which ones I should be looking at (and where in the snapshot I should be looking). For example, this is what I see for 
|
| <th colspan="{n_cols_total}" class="gt_heading gt_title gt_font_normal">{title} | ||
| </tr>""" | ||
| ) | ||
|
|
||
| if has_subtitle: |
There was a problem hiding this comment.
Here, I'm adding the closing </th> tags and also improving somewhat the formatting of the output HTML (makes it easier to visually inspect the generated HTML). Snapshots had to be regenerated for this, though it had to happen anyway because of the addition of closing tags.
| @@ -21,6 +21,10 @@ def create_heading_component_h(data: GTData) -> StringBuilder: | |||
| if not has_title and not has_subtitle: | |||
| return result | |||
|
|
|||
| # Raise an error if there is a subtitle but no title | |||
There was a problem hiding this comment.
Added an additional check here to raise in the case that subtitle text is available in the absence of title text. While this may be also caught within the .tab_header() method, I think it's good to have a second check here in case there is manual addition/removal of title/subtitle (manipulations done outside the prescribed API).
| @@ -154,8 +162,8 @@ def create_columns_component_h(data: GTData) -> str: | |||
| ) | |||
| ) | |||
|
|
|||
| # Join the <th> cells into a string and separate each with a newline | |||
| th_cells = "\n".join([str(tag) for tag in table_col_headings]) | |||
| # Join the <th> cells into a string and begin each with a newline | |||
There was a problem hiding this comment.
An opportunity was taken here to beautify the HTML output of the column labels portion since snapshots had to be regenerated anyway.
|
There are generally quite a few HTML validation problems with the generated output but this PR does address the closing of Before PR changes: 
After: 
I was going to attach a screenshot of the HTML checker results but since there are quite a few problems to be resolved (beyond the scope of this PR) the failure of not having closed |
| <th colspan="4" class="gt_heading gt_title gt_font_normal">Data listing from <strong>exibble</strong> | ||
| <thead class="gt_header"> | ||
| <tr> | ||
| <th colspan="4" class="gt_heading gt_title gt_font_normal">Data listing from <strong>exibble</strong></th> |
There was a problem hiding this comment.
Here is the test that changed, based on this fix. There is a closing </th> element now in this title (and subtitle below).
| <th colspan="4" class="gt_heading gt_subtitle gt_font_normal gt_bottom_border"><code>exibble</code> is a <strong>Great Tables</strong> dataset.</th> | ||
| </tr> | ||
| </thead> | ||
| <tr class="gt_col_headings"> |
There was a problem hiding this comment.
This and the rest of snapshot test changes are just stylistic
|
Thanks, this is super helpful! Based on these notes, I was able to find the snapshot that updated with the added |
There were unclosed tags when generating a header. This generally causes problems when the output HTML is consumed by another process (expectation is that the input HTML should be valid; browsers can handle invalid HTML to great extent, other programs cannot really).
This PR fixes this issue and also performs a few HTML formatting cleanups at the same time. The output HTML was verified in the Nu HTML checker tool (at https://validator.w3.org/nu) to have been improved from the changes here.
Fixes: #224
And this should fix a user issue in quarto-dev/quarto-cli#8971.