-
Notifications
You must be signed in to change notification settings - Fork 1
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
generate styled dataset PDF #1075
Conversation
9ddbc03
to
32e4f18
Compare
9879a4e
to
684eb5c
Compare
684eb5c
to
34b969c
Compare
b498749
to
f7bb6cc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1075 +/- ##
=======================================
Coverage 68.83% 68.83%
=======================================
Files 108 108
Lines 5513 5513
Branches 810 809 -1
=======================================
Hits 3795 3795
Misses 1592 1592
Partials 126 126 ☔ View full report in Codecov by Sentry. |
27ae351
to
9c06034
Compare
c8292bf
to
a8d30a1
Compare
a8d30a1
to
0ecb76e
Compare
e16f93f
to
da37bc1
Compare
da37bc1
to
4378a19
Compare
|
||
|
||
def generate_pdf_from_html(output_html_path: Path, output_pdf_path: Path) -> Path: | ||
def generate_pdf_from_html( | ||
output_html_path: Path, |
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.
change to input_html_path
?
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.
changed all of em to be shorter and consistent with write_oti_xlsx
subprocess.run( | ||
[ | ||
"pandoc", | ||
"weasyprint", |
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.
💪
|
||
|
||
def generate_pdf_from_html(output_html_path: Path, output_pdf_path: Path) -> Path: | ||
def generate_pdf_from_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.
Want to add a typer command for this?
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.
was tempted to but I'd rather do that after considering whether this file generate_metadata_assets.py
and oti_xlsx.py
should even be separate files
if they're worth combining, that'd influence the existing/new typer commands
yaml_file_path = TEST_METADATA_YAML_PATH | ||
output_html_path = TEMP_DATA_PATH / "metadata.html" | ||
output_pdf_path = TEMP_DATA_PATH / "metadata.pdf" | ||
output_xlsx_path = TEMP_DATA_PATH / "my_data_dictionary.pdf" |
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.
hopefully still an xlsx after this refactor?
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.
fixed!
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.
Good stuff! Happy to see weasyprint proven out. A few nits, but no need to re-request review if you get to them.
change in tests seem to have caused "indirect changes" in test coverage
4378a19
to
d56e5ee
Compare
@@ -8,16 +8,21 @@ | |||
DEFAULT_DATA_DICTIONARY_TEMPLATE_PATH = ( | |||
RESOURCES_PATH / "data_dictionary_template.jinja" | |||
) | |||
DEFAULT_DATA_DICTIONARY_STYLESHEET_PATH = RESOURCES_PATH / "data_dictionary.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.
noting that I'm not sure if these DEFAULT_*
variables are the best approach, just wanted to minimize changes in this PR
d56e5ee
to
4bd6a8d
Compare
related to #944, #561
This is a first pass at styling our data dictionary PDFs, not an attempt to use the exact styling that the Design fellows proposed in fIgma.
changes
old vs new PDF
Old
New