posit-dev / great-tables

Make awesome display tables using Python.
https://posit-dev.github.io/great-tables/
MIT License
1.81k stars 60 forks source link

Ensure that header component HTML tags pass HTML validation #235

Closed rich-iannone closed 6 months ago

rich-iannone commented 6 months ago

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: https://github.com/posit-dev/great-tables/issues/224

And this should fix a user issue in https://github.com/quarto-dev/quarto-cli/discussions/8971.

machow commented 6 months ago

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?

machow commented 6 months ago

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 test_formats.ambr. I'm not sure if I'm supposed to scan it manually, or if most of the changes here are just linebreak related):

image
rich-iannone commented 6 months ago

There are generally quite a few HTML validation problems with the generated output but this PR does address the closing of <th> values in the title block. Here are before and after screenshots of the changes.

Before PR changes:

html-before

After:

html-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> is masked by preceding validation failures.

machow commented 6 months ago

Thanks, this is super helpful! Based on these notes, I was able to find the snapshot that updated with the added </th>.