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

Logic for rendering DHCR email content is duplicated #892

Closed
toolness opened this issue Oct 18, 2019 · 0 comments · Fixed by #1520
Closed

Logic for rendering DHCR email content is duplicated #892

toolness opened this issue Oct 18, 2019 · 0 comments · Fixed by #1520

Comments

@toolness
Copy link
Collaborator

In #891 @sraby had to duplicate logic for rendering the DHCR email in both Python and TypeScript.

If sending a rent history request is the only place this kind of thing needs to happen, it's fine, but if we do more of this sort of thing, it would be nice to make things more DRY, e.g. by:

  • Make it possible to have lambda rendering processes render non-HTML content, and have the server delegate the rendering of the email text to the lambda process.

  • Add a GraphQL query or REST endpoint that renders the email content on the server-side and have the front-end use it.

toolness added a commit that referenced this issue Apr 15, 2020
This adds functionality for the React front-end to generate its own "static pages", which consist of completely self-contained HTML that isn't progressively enhanced in any way.

The idea here is for us to be able to create HTML content in our server-side renderer that isn't intended for delivery to a browser, but rather for alternate media such as a PDF (via WeasyPrint) or richly-formatted email.

There's a few reasons we might want to do this:

* It's much easier to write valid HTML in React than it is in Django templates.
* It's easier for our developers to only learn one way of creating HTML content, rather than multiple ways.
* It's easier to unit test our content; currently all our Django templates aren't very well-tested in part because it's not easy to test them, whereas it _is_ straightforward to test React code.
* It's easier to ensure that our HTML-generating code won't raise exceptions, because it's TypeScript whereas Django templates have no type safety.
* We might want to reuse similar components in both alternate media and our website (for example, a function that "prettifies" a U.S. phone number).
* We sometimes duplicate view logic in Python and TypeScript, such as in #892, which this could help us avoid.
* Django templates tend to be tightly coupled to Django models, which makes them harder to reuse and test.  Rendering them React-side might mean a bit more work because it doesn't have direct access to our models, but it could also mean a better separation of concerns and more reusable code.
* We're eventually going to internationalize the site (see #12) and it's very likely that our internationalization solution on the React side will be based on ICU MessageFormat, which results in more user-friendly localization than Django's gettext-based solution.  Also, it's much easier for developers to learn only one internationalization solution.

As a proof-of-concept, this adds a page at `/dev/examples/static-page` that renders a simple static page consisting of the following HTML:

```
<!DOCTYPE html>
<html>
  <meta charSet="utf-8" />
  <title>This is an example static page.</title>
  <p>Hello, this is an example static page&hellip;</p>
</html>
```

Visiting this page in the browser will yield only that HTML and nothing else.
toolness added a commit that referenced this issue Jun 8, 2020
This fixes #892 and tees us up for localizing the email content (#1508).  It also refactors our CSS a bit by adding a new `.jf-email-subject` class that can be used to style the subject lines of our email previews in a DRY way.
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 a pull request may close this issue.

1 participant