Add Support for Django Template Extensions

yuriescl commented 4 years ago

When returning HTML from DepositIntegration.instructions_for_pending_deposit, I'm having to use inline CSS inside the tags, as I couldn't find an easy way to define custom CSS for Polaris templates. Polaris has an integration function to define custom JS, it would be great to have something similar for CSS as well.

UPDATE: This issue now is about two things:

Deprecate integrations that collect data to be used in templates
Add support for django template extensions

The two integrations that serve the purpose template extensions can do better are:

JS scripts() integration function
DepositIntegration.instructions_for_pending_deposit()

content_for_template also returns data for templates but its content that must be sent from the backend, not computed or processed on the frontend.

JakeUrban commented 4 years ago

The instructions_for_pending_deposit() integration needs upgrading. There should be a better interface for providing instructions within the more_info template.

In the immediate future, you can use Polaris' JS script integration. Using this integration you can solve your issue a number of ways better described here.

JakeUrban commented 4 years ago

Any changes to instructions_for_pending_deposit() will be included in a post-v1.1 release. I'm wondering if we should replace the text/html content returned with a Django Template object.

The idea is the Polaris dev would load a HTML Template source file into a Template django object and return it. Tags can reference CSS style groupings defined from a JS script passed via the JS script integration function mentioned above.

If you don't like the idea of returning TemplateScript objects from the JS script integration function and would rather just replace Polaris' CSS styling altogether, see how Polaris allows developers to override the base CSS stylesheet used.

yuriescl commented 4 years ago

I'm wondering if we should replace the text/html content returned with a Django Template object.

Yes. Polaris could support str and Template. If it's str, it treats as plain text and inserts it into more_info.html (same as currently). If it's Template, it renders the template instead of more_info.html. With that change, I would create a template and inherit it from more_info.html. Polaris could, in the base template, add {% block extra_head %} and {% block extra_body %} to the end of <style> and <body> tags respectively, so new templates could easily add their own template-specific styles and scripts without having to override styles.css or adding global JS using the JS integration.

JakeUrban commented 4 years ago

If it's Template, it renders the template instead of more_info.html.

The template returned wouldn't replace more_info.html, it would be inserted into it in the same way the text or HTML is inserted today.

JakeUrban commented 4 years ago

See how Polaris' deposit template is loaded into the base template.

yuriescl commented 4 years ago

If it's Template, it renders the template instead of more_info.html.

The template returned wouldn't replace more_info.html, it would be inserted into it in the same way the text or HTML is inserted today.

I see. But still, inserting custom CSS (including a new CSS file, for example) should not be difficult. Right now it would require either using JS or overriding styles.css entirely, correct?

yuriescl commented 4 years ago

Just adding the extra_ tags would make it far easier to write new templates. This would maybe even remove the need for a JS integration since the the developer could just inherit base.html and just add his <script> tags inside extra_body.

JakeUrban commented 4 years ago

But still, inserting custom CSS (including a new CSS file, for example) should not be difficult. Right now it would require either using JS or overriding styles.css entirely, correct?

Thats correct. You can override base.css entirely but you can also just make a copy and build/modify it.

JakeUrban commented 4 years ago

Lets clear something up about how the Template stuff will work for instructions_for_pending_deposit().

You are not going to be writing your own Template that inherits from base.html, you'll be writing a Template that is rendered within more_info.html.

When instructions_for_pending_deposit() is called, a Template object will be returned instead of text. This instructions_template will be rendered within more_info.html like so:

% block "content" %}
<section class="section receipt">
    {% if instructions_text is not none %}
    <div class="info-item">
        <div class="info-label">
            {% trans "instructions" %}
        </div>
        <div class="field-value">
            {{ instructions|safe }}
        </div>
    </div>
    {% if instructions_template is not none %}
    {% block "instructions_template" %}
    {% endblock %}
    {% endif %}

This instructions_template will contain rendered HTML code to insert into that page. See the more_info.html page to get a better understanding of how it works today.

The JS script integrations will likely not be removed. It is important to load JS scripts defined by the anchors within the body of the HTML doc, not the head. Thats why it makes more sense to provide a new integration that allows Polaris developers to put custom CSS link tags in the <head> tags, separate from the <script> tags loaded by the JS integration at the bottom of the body.

yuriescl commented 4 years ago

Understood. I'm not sure if I agree with managing CSS/JS inside Python code (integration functions). That's why I was suggesting the {% extra_head %} and {% extra_body %} tags and so on. There should be a better way of doing this, I'll get back to you if I have any ideas regarding that.

JakeUrban commented 4 years ago

Actually, we can do this today via content_for_template(). I could allow a stylesheets key that would map to a list of Polaris TemplateLink (think CSS TemplateScript) objects. These TemplateLink objects would contain the file paths to the CSS source files listed in their static directory and be rendered as <link href=../> in whatever template is being rendered.

JakeUrban commented 4 years ago

So to review, there are two separate issues being discussed here:

You'd like to have a better way of specifying custom CSS (as opposed to overriding base.css)
You'd like to have better template support for deposit instructions

Hopefully both of these issues are resolves by:

Adding this stylesheets key to content_for_template() and TemplateLink object, which should allow you to add CSS to the head of the HTML doc per-page displayed.
Adding a instructions_template variable to the more_info.html template that will contain rendered HTML content passed as a Template object by the developer from instructions_for_pending_deposit()

JakeUrban commented 4 years ago

Looks like I didn't see this comment before posting the 2 comments above, does my improved solution make sense?

yuriescl commented 4 years ago

Here's an idea:

Modify base.html by adding extra_head and extra_body tags:


...
    background-repeat: no-repeat;
    background-position: right 1em top 50%, 0 0;
  }
</style>

  {% block extra_head %}
  {% endblock %}
</head>

...

  {% endif %}
{% endfor %}

{% block extra_body %} {% endblock %}


- Remove the JS integration function
- Move Polaris templates to a subfolder called `polaris` to have a structure like:
  * `templates/polaris/base.html`
  * `templates/polaris/transaction/...`
  * This avoids conflicts the Anchor templates and is a common practice for static files as well
- Update documentation suggesting that if the Anchor needs to add CSS or JS to templates, it can do it by overriding the template.  
For example, let's say the Anchor wants to add styles or scripts to `more_info.html` **only**.  
It could do it by creating a template in `templates/polaris/transaction/more_info.html`:

{% extends "polaris/transaction/more_info.html" %}

{% block extra_head %} [...custom styles...] [whatever else the Anchor wants at the end of ] {% endblock %}

{% block extra_body %}

[whatever else the Anchor wants at the end of ] {% endblock %}



This allows the Anchor to override specific templates instead of loading global JS/CSS in all pages, which is the case with the current integration function.

yuriescl commented 4 years ago

Looks like I didn't see this comment before posting the 2 comments above, does my improved solution make sense?

It does make sense but is a workaround for the already existing method, which I think is not optimal.

JakeUrban commented 4 years ago

I really like your idea, but it would be a breaking change to Polaris, which something I want to avoid until 2.0. I'll create the 2.0 milestone now and begin logging breaking changes we want to make, including this one (update: here).

If possible, I'd like to stick with our current mechanisms. However, if the other major stakeholders in Polaris want this change as well and are willing to update their implementations due to the breaking change, then we could get started on 2.0 now.

I would suggest making a post to the Polaris Google Group, which has the major Polaris stakeholders subscribed.

At the moment I don't have the bandwidth to do this change, regardless of if its 1.2 or 2.0. Depending on your needs and timeline, you're more than welcome to implement 1.2 now or 2.0 if everyone is on board.

JakeUrban commented 4 years ago

I hope it makes sense why I want to get major stakeholder buy-in before making a breaking change. Others need to continue updating Polaris when Protocol changes happen, including when 14 comes out. If the next version of Polaris is a breaking change, it's more work the developer must do immediately.

JakeUrban commented 4 years ago

https://github.com/stellar/django-polaris/issues/318

yuriescl commented 4 years ago

We could add the advanced template support in 1.2 and keep the JS integration as it is. In 2.0 we could deprecate the integration function and remove it later in 2.x.

yuriescl commented 4 years ago

I'll open a discussion in the Google Group.

yuriescl commented 4 years ago

However I do think moving the templates to a polaris subfolder would be a priority. It's not a breaking change since Anchors are not likely to be overriding templates at the moment.

JakeUrban commented 4 years ago

Thanks! I actually like that idea of deprecation. As long as the 2.0 changes can be made in a way that isn't disruptive to the current system I would be alright with that. If conversation picks up in the google group we can continue there, otherwise lets go to the new issue for this problem.

That way, we can continue discussing the deposit instructions fix here. Can I change the title of this issue?

yuriescl commented 4 years ago

I agree, yes.

JakeUrban commented 4 years ago

However I do think moving the templates to a polaris subfolder would be a priority. It's not a breaking change since Anchors are not likely to be overriding templates at the moment.

We already have the following templates directory structure: Screen Shot 2020-09-28 at 1 19 11 PM

But I see you mean you'd like to see something like so:

templates/

django/
- ...
polaris/
- deposit/
  - form.html
- transaction/
  - more_info.html
- widgets/
  - ...
- withdraw/
  - form.html
- base.html
- error.html

yuriescl commented 4 years ago

Yes

JakeUrban commented 4 years ago

I think we can do that in 1.2 and just let the strategic anchors know via the group.

JakeUrban commented 4 years ago

I think I'm going to use the solution you outlined for extra_head/extra_body but for the instructions.

Developers will define an instructions block, instead of returning something from a instructions_for_pending_deposits() function. I think I'll be adding the extra_head/extra_body blocks too.

The docs will have to be updated to list what variables are passed to each template.

yuriescl commented 4 years ago

I agree, HTML/CSS/JS should not be inside .py files.

JakeUrban commented 4 years ago

The Django 2.2 docs don't have this docs section but it looks like template extensions work still, so thats good.

I'm guessing 2.2 has the same functionality but didn't have the docs section.

yuriescl commented 4 years ago

I've tested on 2.2 and it works.

stellar / django-polaris

Add Support for Django Template Extensions #317