3
\$\begingroup\$

I have attempted to hack together a CV/Resume builder and I would like a review. The aim is to output a styled PDF containing dynamic fields. I have taken a simple HTML file, an arguably bloated CSS file and the below test dataframe data from online.

Here is the code:

from __future__ import annotations 

from typing import ClassVar, Any

import jinja2
import pandas as pd
from random import getrandbits, randint

import pdfkit
from jinja2 import Template


# Just random data

def random_hex(length=10):
    return '%0x' % getrandbits(length * 4)


COLS = ['number', 'name', 'data1', 'data2', 'data3']
df = pd.DataFrame(
    [
        {
            "number": randint(0, 100),
            "name": random_hex(15),
            "data1": random_hex(5),
            "data2": random_hex(5),
            "data3": random_hex(5)
        }
        for i in range(10)
    ]
)[COLS]


class CVBuilder:

    TEMPLATE_DIR: ClassVar[str] = "templates"
    TEMPLATE_NAME: ClassVar[str] = "cv_template.html"
    OUTPUT_DIR: ClassVar[str] = "output.pdf"
    CSS_FILE: ClassVar[str] = "cv_template.css"

    def __init__(self, cv_content: Any):  # TODO
        self.content = cv_content

    @classmethod
    def cv_template(cls) -> Template:
        environment = jinja2.Environment(
            loader=jinja2.FileSystemLoader(cls.TEMPLATE_DIR)
        )
        template = environment.get_template(cls.TEMPLATE_NAME)
        return template

    @classmethod
    def build_cv(cls, **kwargs) -> CVBuilder:
        cv_template = cls.cv_template()
        return cls(cv_template.render(**kwargs))

    def to_pdf(self) -> None:
        pdfkit.from_string(
            input=self.content,
            output_path=self.OUTPUT_DIR,
            css=self.CSS_FILE
        )


if __name__ == "__main__":
    content = df.to_html(index=False, classes="table-title", border=False)
    cv = CVBuilder.build_cv(
        df=content,
        title="Example",
        message="This is an example text input"
    )
    cv.to_pdf()
  • I think I could probably nuke the ClassVar stuff
  • I think I could probably use a dataclass
  • Do I even need a class?

Here is the HTML and CSS:

<link rel="stylesheet" type="text/css" href="cv_template.css"/>
<html lang="en">
<head>
    <meta charset="utf-8"/>
    <title>{{title}}</title>
    <meta name="viewport" content="initial-scale=1.0; maximum-scale=1.0; width=device-width;">
</head>
<body>
    <div class="table-title" align="center">
        <h4>{{ title }}</h4>
        <p style="font-weight: bold">templating example.</p>
    </div>
    <div align="center">
        {{ df }}
    </div>
    <p align="center">{{ message }}</p>
</body>
</html>
/*** Most of this CSS is unused in the exaple  **/
@import url(https://fonts.googleapis.com/css?family=Roboto:400,500,700,300,100);
body {
  background-color: #ffffff;
  font-family: "Roboto", helvetica, arial, sans-serif;
  font-size: 22px;
  font-weight: 400;
  text-rendering: optimizeLegibility;
}

h4 {
    margin-bottom: 0px;
    margin-top: 5px;
    }

hr {
    height:2px;
    visibility:hidden;
    line-spacing:0px;
}

div.table-title {
   display: block;
  margin: auto;
  max-width: 600px;
  padding:5px;
  width: 100%;
}

.table-title h3 {
   color: #000000;
   font-size: 30px;
   font-weight: 400;
   font-style:normal;
   font-family: "Roboto", helvetica, arial, sans-serif;
   text-shadow: -1px -1px 1px rgba(0, 0, 0, 0.1);
   text-transform:uppercase;
}


/*** Table Styles **/

.table-fill {
  background: white;
  border-radius:3px;
  border-collapse: collapse;
  border-bottom:2px solid #C1C3D1;
  border-right:2px solid #C1C3D1;
  border-left:2px solid #C1C3D1;
  height: 320px;
  margin: auto;
  max-width: 600px;
  padding:5px;
  width: 100%;
  box-shadow: 0 5px 10px rgba(0, 0, 0, 0.1);
  animation: float 5s infinite;
}

th {
  color:#D5DDE5;
  background:#1b1e24;
  border-bottom:2px solid #1b1e24;
  border-top:2px solid #1b1e24;
  border-right: 1px solid #343a45;
  font-size:23px;
  font-weight: 300;
  padding:12px;
  text-align:left;
  text-shadow: 0 1px 1px rgba(0, 0, 0, 0.1);
  vertical-align:middle;
  white-space: pre;
}

th:first-child {
  border-top-left-radius:3px;
  border-left: 2px solid #1b1e24;
}

th:last-child {
  border-top-right-radius:3px;
  border-right: 2px solid #1b1e24;
}

tr {
  border-top: 1px solid #C1C3D1;
  border-bottom-: 1px solid #C1C3D1;
  color:#000000; /*#666B85*/
  font-size:16px;
  font-weight: normal;
  text-shadow: 0 1px 1px rgba(256, 256, 256, 0.1);
  white-space: pre;
}

tr:hover td {
  background:#4E5066;
  color:#FFFFFF;
  border-top: 1px solid #22262e;
  border-bottom: 1px solid #22262e;
}

tr:first-child {
  border-top:none;
}

tr:last-child {
  border-bottom:none;
}

tr:nth-child(odd) td {
  background:#EBEBEB;
}

tr:nth-child(odd):hover td {
  background:#4E5066;
}

tr:last-child td:first-child {
  border-bottom-left-radius:3px;
}

tr:last-child td:last-child {
  border-bottom-right-radius:3px;
}

td {
  background:#FFFFFF;
  padding:20px;
  text-align:left;
  vertical-align:middle;
  font-weight:450;
  font-size:20px;
  text-shadow: -1px -1px 1px rgba(0, 0, 0, 0.1);
  border-right: 1px solid #C1C3D1;
  white-space: nowrap;
}

td:last-child {
  border-right: 0px;
}

th.text-left {
  text-align: left;
}

th.text-center {
  text-align: center;
}

th.text-right {
  text-align: right;
}

td.text-left {
  text-align: left;
}

td.text-left-top {
  vertical-align: top;
  text-align: left;
}

td.text-center {
  text-align: center;
}

td.text-right {
  text-align: right;
}

footer {
  page-break-after: always;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I agree that the CSS may be a bit bloated. That may be unavoidable when you want very specific settings but the CSS is bigger than your HTML template and that is a sign isn't it.

I would normally define a font family and a default font size at the BODY level, so that contained elements inherit from it. Then, only override when needed.

Your body section contains:

  font-family: "Roboto", helvetica, arial, sans-serif;
  font-size: 22px;
  font-weight: 400;

Then, it should not be necessary to repeat the font for style .table-title h3.

Omit default settings

font-weight: 400; is the same as: font-weight: normal; and should be the default setting applied by the browser anyway, so it seems to me that it can be safely omitted.

Remove unused styles

Some styles seem not be used at present, for example:

td.text-left-top {
  vertical-align: top;
  text-align: left;
}

And perhaps this could be merged with your TH definition ?

But you already know that very well, since you've actually written that in comments.

Your current template is very minimalistic. But you still have a lot of inline styles for a small template, which defeats the purpose of using a CSS file. Examples:

<p style="font-weight: bold">templating example.</p>

or:

<div align="center">

Add CSS styles for those, and get rid of what you don't need. You can always put the styles back in place as you need them, but one by one.

Fixed vs relative size

I would discourage using fixed pixel size for text, because some screens have a higher resolution and different DPI values. The rendering will vary from one screen to another. Prefer relative size for responsive layout. It doesn't hurt to set a minimum value though.

I am not even certain that the contents will always fit inside your tables if you increase the amount of text. Either the browser constrains the content, and part of it shall remain invisible, or the browser stretches the layout as necessary and then your height specification is neutered. Test extensively. Use your desktop computer, a mobile phone, a tablet, reduce window size and see what happens. Try zooming too.

Suggested reading: Pixels vs. Relative Units in CSS: why it’s still a big deal

Shorthand notation

You can reduce the number of lines slightly by using shorthand notation. For example:

.table-fill {
  background: white;
  border-radius:3px;
  border-collapse: collapse;
  border-bottom:2px solid #C1C3D1;
  border-right:2px solid #C1C3D1;
  border-left:2px solid #C1C3D1;
  height: 320px;
  margin: auto;
  max-width: 600px;
  padding:5px;
  width: 100%;
  box-shadow: 0 5px 10px rgba(0, 0, 0, 0.1);
  animation: float 5s infinite;
}

Since 3 of the 4 borders have the same style and only the top border is different you could write:

  border: 2px solid #C1C3D1;
  border-top: 0;

This is effectively an override.

Typo

In TR definition (a trailing hyphen)

border-bottom-: 1px solid #C1C3D1;

Spacing

In the very next lines you have:

  color:#000000; /*#666B85*/
  font-size:16px;

Add some spacing to have a consistent property: value coding style throughout your CSS definition (at least I find that slightly more readable).

HTML

The HTML is broken - the <link rel="stylesheet" should be within the HEAD section:

<link rel="stylesheet" type="text/css" href="cv_template.css"/>
<html lang="en">
<head>
    <meta charset="utf-8"/>
    <title>{{title}}</title>
    <meta name="viewport" content="initial-scale=1.0; maximum-scale=1.0; width=device-width;">
</head>

Since broken HTML (and CSS) code has always been very common, browsers tend to be very forgiving but may not all react the same way. Rendering may be inconsistent from one browser to another. So you test against as many browsers as possible and in different resolutions.

Accessibility

OK, I am not the expert and I am guilty of reusing old code without giving it much thought too. But from time to time reviewing code compels me to check the relevant documentation. Your HTML page has the following viewport definition:

<meta name="viewport" content="initial-scale=1.0; maximum-scale=1.0; width=device-width;">

This may be bad in line of what's been said already but so that you understand the possible implications (emphasis is mine) :

Maximum-scale is designed to prevent the user from zooming too far in on a page, and is not intrinsically bad, however it is relatively easy to accidentally set up the viewport so that the maximum-scale disables using from zooming at all, so it is generally not recommended. For this reason, Apple decided to ignore the declarations of user-scalable, minimum-scale, and maximum-scale, as of iOS 10.

The recommended, 'safe' viewport setting is:

<meta name="viewport" content="width=device-width, initial-scale=1">

Source

Test, test, test, torture your layout until it behaves well in every condition :)

Final notes

I have not yet reviewed the Python and the Jinja proper, which are probably more important to you. Should you use a dataclass ? You could, but the benefit would be inconsequential I think. Do you even need a class ? At this point I tend to say no because you have just 3 methods and the class does not perform a lot of abstraction.

If on the other hand you added the possibility to embed a picture, which is common in résumés, this could get interesting. If you wanted to export to different formats like MS Word, then the case for a class becomes stronger. If this is something you would be interested in, read up about Python interfaces, and this article may be a good introduction: The Factory Method Pattern and Its Implementation in Python

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.