-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: add google font helper, implement in opt_table_font()
and style.text()
#423
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
+ Coverage 86.75% 86.78% +0.03%
==========================================
Files 42 42
Lines 4702 4745 +43
==========================================
+ Hits 4079 4118 +39
- Misses 623 627 +4 ☔ View full report in Codecov by Sentry. |
rendered = "" | ||
|
||
if self.color: | ||
rendered += f"color: {self.color};" | ||
if self.font: | ||
rendered += f"font-family: {self.font};" | ||
font = self.font |
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.
Can you test the lines here that are flagged as untested in codecov?
if any(isinstance(s, CellStyle) for s in style): | ||
|
||
for s in style: | ||
if ( | ||
isinstance(s, CellStyle) | ||
and hasattr(s, "font") | ||
and s.font is not None | ||
and isinstance(s.font, GoogleFont) | ||
): | ||
# Obtain font name and import statement as local variables | ||
font_name = s.font.get_font_name() | ||
font_import_stmt = s.font.make_import_stmt() | ||
|
||
# Replace GoogleFont class with font name | ||
s.font = font_name | ||
|
||
# Append the import statement to the `table_additional_css` list | ||
existing_additional_css = self._options.table_additional_css.value + [ | ||
font_import_stmt | ||
] | ||
|
||
# Add revised CSS list via the `tab_options()` method | ||
new_data = new_data.tab_options(table_additional_css=existing_additional_css) |
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.
For pairing Thursday, let's go over alternative paths this could go through. Currently, CellStyle as an interface has a _to_html_style()
method, that gets used on render.
Here, it looks like the new behavior to support is this:
- A certain type of style requires setting some initial table-level CSS (once for the importing of fonts)
Currently, the path used for this is:
- in tab_style, detect a particular CellStyleText (e.g. with a google font)
- create import statement for font
- set initial table css for importing font in the additional_css option
- additional css gets included on render
However, this means that if you use tab_style multiple times with google font, etc.., the additional css gets appended multiple times. Moreover, if the person modifies additional_css, they can remove the necessary import.
A second issue is that this exact code is pasted into the opt_table_font()
method. We likely don't want to put custom handling for specific CellStyle types (with specific attributes) into these methods.
Repeating code twice isn't necessarily an issue though, so it could be that we map out paths for these kinds of activities, merge, and punt the work to a new issue
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.
Sounds good!
# Ensure that list items in `additional_css` are unique and then combine statements while | ||
# separating with `\n`; use an empty string if list is empty or value is None | ||
if has_additional_css: | ||
additional_css_unique = OrderedSet(additional_css).as_list() |
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 ensures that if google_font()
is used multiple times, that it doesn't keep adding css
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; from pairing, we have an issue open for exposing a general mechanism for google font-like behaviors, and will punt it down the road
This adds support for incorporating Google fonts into the table text, via:
opt_table_font(font=google_font(...))
tab_style(style=style.text(font=google_font(...)
The necessary additions are:
table_additional_css=
argument intab_options()
google_font()
functioncompile_scss()
to add the necessary@import
statement at the top of the<style>
block in an HTML table renderA simple example of this is through the use of
opt_table_font()
Fixes: #374