-
Notifications
You must be signed in to change notification settings - Fork 103
feat: add the tab_footnote()
method
#763
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
==========================================
+ Coverage 91.45% 91.72% +0.27%
==========================================
Files 47 47
Lines 5558 5875 +317
==========================================
+ Hits 5083 5389 +306
- Misses 475 486 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
great_tables/_utils_render_html.py
Outdated
from ._spanners import spanners_print_matrix | ||
from ._tbl_data import _get_cell, cast_frame_to_string, replace_null_frame | ||
from ._text import BaseText, _process_text, _process_text_id | ||
from ._utils import heading_has_subtitle, heading_has_title, seq_groups | ||
|
||
# Visual hierarchy mapping for footnote location ordering | ||
FOOTNOTE_LOCATION_HIERARCHY = { |
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.
Please do not hard-code any mappings from strings to information. Either add it as an attribute on the class, or use a function / mapping from dataclasses to info.
great_tables/_utils_render_html.py
Outdated
footnote_positions: list[tuple[tuple[int, int, int], FootnoteInfo]] = [] | ||
|
||
for fn_info in visible_footnotes: | ||
if fn_info.locname == "none": |
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 seems concerning, that there is a string named "none"
great_tables/_gt_data.py
Outdated
@@ -875,10 +875,10 @@ class FootnotePlacement(Enum): | |||
|
|||
@dataclass(frozen=True) | |||
class FootnoteInfo: | |||
locname: Loc | None = None | |||
locname: str | None = None |
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.
From pairing, this should be changed back to Loc (which will require fairly extensive refactoring in areas where locname strings are checked)
great_tables/_locations.py
Outdated
@@ -1088,4 +1088,144 @@ def _(loc: None, data: GTData, footnote: str, placement: PlacementOptions) -> GT | |||
|
|||
@set_footnote.register | |||
def _(loc: LocTitle, data: GTData, footnote: str, placement: PlacementOptions) -> GTData: | |||
raise NotImplementedError() | |||
place = FootnotePlacement[placement] | |||
info = FootnoteInfo(locname="title", footnotes=[footnote], placement=place, locnum=1) |
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.
locname should just be the loc dataclass instance. (see style loc handling)
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.
It seems like currently, the code inserts strings for locations. From pairing, it seems like the footnotes location code is largely the same as that for styling (e.g. targetting footnote locations). Let's explore consolidating the code to set style and footnote data on the GT object.
_locations.py example:
Here is an example modification to set_style()
for LocBody()
that handles both footnote and style entries. Note the addition of FootnoteEntry
.
@set_style.register
def _(loc: LocBody, data: GTData, style: list[CellStyle | FootnoteEntry]) -> GTData:
positions: list[CellPos] = resolve(loc, data)
# CHANGE MADE HERE: split styles and footnotes -----
style, new_footnotes = footnotes_split_style_list(style)
# ENDCHANGE
# evaluate any column expressions in styles
style_ready = [entry._evaluate_expressions(data._tbl_data) for entry in style]
all_info: list[StyleInfo] = []
for col_pos in positions:
row_styles = [entry._from_row(data._tbl_data, col_pos.row) for entry in style_ready]
crnt_info = StyleInfo(
locname=loc, colname=col_pos.colname, rownum=col_pos.row, styles=row_styles
)
all_info.append(crnt_info)
# CHANGE MADE HERE: added _footnotes ----
return data._replace(_styles=data._styles + all_info, _footnotes=data._footnotes + new_footnotes)
Then, basically anywhere styles are handled when rendering HTML, there could be a related footnote handling (I think this currently happens in the PR).
Big changes needed
- No use of strings for loc (i.e. us isinstance checks or whatever style does)
- No priority info separate from Loc data (e.g. add attribute to Loc with priority level; OR define function that takes a loc and returns a priority based on instance checks / dispatching, etc.. but not strings.)
@@ -285,7 +324,14 @@ def create_columns_component_h(data: GTData) -> str: | |||
level_1_spanners.append( | |||
tags.th( | |||
tags.span( | |||
HTML(_process_text(spanner_ids_level_1_index[ii])), | |||
HTML( |
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.
Logic like this should match styles logic (above) for filtering FootnoteInfo. Notice that on line 317 (style_i = ...
), the filtering is done directly. We should match this behavior for footnotes (rather than passing down into a function). If we change it for one, we should change for both.
Recommend pulling filtering out of _add_footnote_marks_to_text()
and filtering directly with equivalent styles_i = ...
logic (if that's sensible).
great_tables/_utils_render_html.py
Outdated
return 0 | ||
|
||
|
||
def _add_footnote_marks_to_text( |
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.
AFAICT it would be helpful if this function did not do any filtering of the FootnoteInfo. That way, similar to how styles are handled, people would see the footnote filtering is very similar, and then a simple function call similar to _flatten_styles()
illustrating how footnote text gets inserted or something.
This PR adds the
tab_footnote()
method which allows you to add footnotes for different locations in the table. The method integrates footnote mark placement into the rendering pipeline for table headings, column labels, body cells, etc., and extends the internal data structures and options to support footnote config.Here's an example of how this works in practice:
And here is how it looks:
Fixes: #164