-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve HTML representation of tables #233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests in this PR. Do you mind adding some?
From us pairing, it seems useful to have some machinery to mock environments. That way we can sanity check what gets rendered in different places.
great_tables/gt.py
Outdated
if context == "html" and make_page: | ||
html_table = built_table._render_as_html( | ||
make_page=make_page, | ||
page_background_color=page_background_color, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if page_background_color should be an option (e.g. in tab_options()
)
great_tables/gt.py
Outdated
<html lang="en"> | ||
<head> | ||
<meta charset="utf-8"/> | ||
<style>body {{ background-color:{page_background_color};}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a screenshot in another place, but it seems like we'd need a more targeted style here. E.g. we could give body an id or something similar.
dc6cdb0
to
a0cfe36
Compare
From pairing w/ Rich, I've consolidated the code for inferring the IDE environment into the (Rich mentioned he'll pick it up from here!) |
assert snapshot == html_repr_output | ||
|
||
|
||
@mock.patch.dict(os.environ, {"QUARTO_BIN_PATH": "1"}, clear=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good -- the one big note I left is that the css style setting body background to white is too broad--it can affect the entire page (similar to the screenshot I posted that turned the page blue).
If it can be removed, then feel free to remove and merge. (You'll know better than me whether that's possible ;)
great_tables/gt.py
Outdated
<html lang="en"> | ||
<head> | ||
<meta charset="utf-8"/> | ||
<style>body {{ background-color:white; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This styles every body element on the page to white (including the overall body element for the page). We need a more specifically scoped css style. If we want to set only the body element defined here to white, then we need to either...
- specify the style inline
- give the body element a unique id and set the style on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't go all the way with this. I'm going to remove it entirely.
@isabelizimm @machow thank you for your help with this PR! Going to merge now but there should be many more improvements in this particular area down the road. |
This PR adds the option to generate a complete HTML page around the emitted table fragment. And this vastly improves the printing of a table in the vscode jupyter notebook environment by raising the specificity of the tables CSS rules (in the
<style>
block). We also detect whether rendering is occurring in a Quarto document and, in that case, the compiled CSS is kept as is.Fixes: #228