Skip to content
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

Merged
merged 20 commits into from
Sep 23, 2024

Conversation

rich-iannone
Copy link
Member

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:

  • the table_additional_css= argument in tab_options()
  • the google_font() function
  • statements in compile_scss() to add the necessary @import statement at the top of the <style> block in an HTML table render

A simple example of this is through the use of opt_table_font()

from great_tables import GT, exibble, google_font

GT(exibble.head()).opt_table_font(font = google_font("IBM Plex Mono"))
image

Fixes: #374

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 89.79592% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.78%. Comparing base (49ceeb7) to head (a043032).
Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
great_tables/_styles.py 66.66% 3 Missing ⚠️
great_tables/_helpers.py 90.90% 1 Missing ⚠️
great_tables/_options.py 91.66% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot temporarily deployed to pr-423 August 27, 2024 18:36 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 27, 2024 19:07 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 28, 2024 18:15 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 28, 2024 20:41 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 28, 2024 21:23 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 29, 2024 16:32 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 29, 2024 20:43 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 August 29, 2024 20:52 Destroyed
rendered = ""

if self.color:
rendered += f"color: {self.color};"
if self.font:
rendered += f"font-family: {self.font};"
font = self.font
Copy link
Collaborator

@machow machow Sep 12, 2024

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?

here it is in codecov

@github-actions github-actions bot temporarily deployed to pr-423 September 17, 2024 18:03 Destroyed
@github-actions github-actions bot temporarily deployed to pr-423 September 17, 2024 18:55 Destroyed
Comment on lines +126 to +148
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)
Copy link
Collaborator

@machow machow Sep 18, 2024

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

Copy link
Member Author

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()
Copy link
Collaborator

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

Copy link
Collaborator

@machow machow left a 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

@rich-iannone rich-iannone merged commit b14c8f2 into main Sep 23, 2024
13 checks passed
@rich-iannone rich-iannone deleted the add-google-font-helper branch September 23, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add google_font()
2 participants