-
Notifications
You must be signed in to change notification settings - Fork 77
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
Allow for granular section restyling via convenience api #341
Conversation
d1e303d
to
ee9d265
Compare
ee9d265
to
ed23b2c
Compare
Hey, thanks for this incredibly comprehensive PR! At first I was nervous about the idea of a PR implementing all locations at once, but after seeing your changes, it seems very reasonable! Happy to kick the tires, help with anything :) |
ed23b2c
to
4d3f502
Compare
4d3f502
to
eebc708
Compare
eebc708
to
e578e66
Compare
e578e66
to
3b2f0d0
Compare
04788fa
to
26cd5ad
Compare
26cd5ad
to
5c18797
Compare
I'm going to try and complete this PR this week, if that's okay! It's looking really good, and I think will be nice to have for WIP on rendering GT objects interactively (in reactable-py) 😁 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #341 +/- ##
==========================================
+ Coverage 87.14% 87.54% +0.39%
==========================================
Files 42 42
Lines 4768 4840 +72
==========================================
+ Hits 4155 4237 +82
+ Misses 613 603 -10 ☔ View full report in Codecov by Sentry. |
🚀 |
@@ -610,7 +611,7 @@ def order_groups(self, group_order: RowGroups): | |||
# TODO: validate | |||
return self.__class__(self.rows, self.group_rows.reorder(group_order)) | |||
|
|||
def group_indices_map(self) -> list[tuple[int, str | None]]: | |||
def group_indices_map(self) -> list[tuple[int, GroupRowInfo | 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.
now returning GroupRowInfo directly, since we need the group ids, not just their defaulted labels to match group name locs
@@ -869,8 +870,7 @@ class FootnoteInfo: | |||
|
|||
@dataclass(frozen=True) | |||
class StyleInfo: | |||
locname: str | |||
locnum: int | |||
locname: Loc |
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 with a Loc class now fully identifies a style info, so no need for locnum
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.
Great!
@singledispatch | ||
def set_style(loc: Loc, data: GTData, style: list[str]) -> GTData: | ||
"""Set style for location.""" | ||
raise NotImplementedError(f"Unsupported location type: {type(loc)}") | ||
|
||
|
||
@set_style.register(LocHeader) |
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.
Since we removed .locnum and just use the Loc isntances directly, we were able to consolidate most simple versions of set_style()
def _(loc: LocRowGroups, data: GTData) -> set[int]: | ||
# TODO: what are the rules for matching row groups? | ||
# TODO: resolve_rows_i will match a list expr to row names (not group names) | ||
group_pos = set(pos for _, pos in resolve_rows_i(data, loc.rows)) |
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.
Note that this currently can't match row groups by their name (which is also their id). It can only match by index number or match every group.
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.
Totally okay since we don't currently make it possible for the user to set nice IDs anyhow.
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.
LGTM!
Shoot, @timkpaine I accidentally did a squash merge, but this doesn't let me see which lines I changed in the blame (so I can scold myself for any dumb changes I made). I reverted the merge, but apparently you can't reopen a merged PR :/. Any chance you might be able to open a new PR against this repo again(can do it from |
Summary
This is a work-in-progress PR for allowing more granular styling of the various components via the API currently used for cell styling (e.g.
loc.body()
). The motivation is that it's often desired to style e.g. all column labels at once. It looks like this was already started in part (there are many TODOs such as these).This PR is nowhere near done yet, so please don't approve CI running (it will just make noise).
Early Example:
Checklist