PERF: only output an html id if a style is applied #23019
Conversation
|
Hello @Moisan! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23019 +/- ##
==========================================
+ Coverage 92.2% 92.2% +<.01%
==========================================
Files 169 169
Lines 50888 50892 +4
==========================================
+ Hits 46919 46923 +4
Misses 3969 3969
Continue to review full report at Codecov.
|
|
I think this should be a parameter given to the Styler constructor, not something that happens implicitly. |
TomAugspurger
added
Output-Formatting
pandas/io/formats/style.py
Outdated
| a unique identifier to avoid CSS collisions; generated automatically | ||
| caption: str, default None | ||
| caption to attach to the table | ||
| all_ids: bool, default False |
There was a problem hiding this comment.
Perhaps cell_ids is a bit more descriptive?
There was a problem hiding this comment.
And the default should be True (backwards compatible)
pandas/io/formats/style.py
Outdated
| caption: str, default None | ||
| caption to attach to the table | ||
| all_ids: bool, default False | ||
| if True, each cell will have an ``id`` attribute in their HTML tag. |
There was a problem hiding this comment.
Capitalize if.
Could you include the form these take? I believe it's T_<uuid>_row<i>_col<j> for values in the table.
pandas/io/formats/style.py
Outdated
| "display_value": formatter(value), | ||
| "is_visible": (c not in hidden_columns)} | ||
| # only add an id if the cell has a style | ||
| if self.all_ids or \ |
There was a problem hiding this comment.
This should just be if self.cell_ids I think. We don't want a special case for the number of styles.
pandas/io/formats/templates/html.tpl
Outdated
| <tr> | ||
| {%- for c in r %} | ||
| {%- if c.is_visible != False %} | ||
| {%- if c.id: %} |
There was a problem hiding this comment.
I'm not that familiar with jinja, but presumably there's a way to deduplicate this a bit? I worry about these two cases drifting apart in the future.
| </tr> | ||
| {%- endblock tr %} | ||
| {% block before_rows %}{% endblock before_rows %} | ||
| {% for r in body %} |
There was a problem hiding this comment.
Does removing the opening - change the whitespace? Or was it redundant?
There was a problem hiding this comment.
I believe it was redundant.
TomAugspurger
left a comment
TomAugspurger
left a comment
There was a problem hiding this comment.
Just removed a backslash.
Ping on green.
|
@TomAugspurger ping |
* PERF: only output an html id if a style is applied * Add a parameter to Styler constructor * Use cell_ids instead of all_ids and set default to True * remove backslash
3 participants
git diff upstream/master -u -- "*.py" | flake8 --diffGreatly reduces the amount of
idtag in the cells by only assigning one when a style is applied to that cell. If that is the not correct approach for solving #20695, I am open to suggestions.Also I was confused by the
%-tags to handle the whitespaces. Is there a reason the default options to handle whitespace are not used?