Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add context arguments to TemplateColumn #509

Merged
merged 5 commits into from
Dec 5, 2017
Merged

Add context arguments to TemplateColumn #509

merged 5 commits into from
Dec 5, 2017

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Dec 4, 2017

Hello,

I suggest add argument to TemplateColumn. This is important to ensure effective re-use of templates as in following example.

Example template:

{% load i18n %}
<div class="btn-group" role="group" aria-label="{{ record }} actions">
    <a href="{% url update_viewname pk=record.pk %}" class="btn btn-success">
        {% trans 'Update' %}
    </a>
    <a href="{% url delete_viewname pk=record.pk %}" class="btn btn-danger">
        {% trans 'Delete' %}
    </a>
</div>

Example table:

class PhoneNumberTable(tables.Table):
    action = TemplateColumn(template_name='auth_factories/_default/_action_table.html',
                            context={'update_viewname': 'auth_factories:sms_code:update',
                                     'delete_viewname': 'auth_factories:sms_code:delete'})

    class Meta:
        model = PhoneNumber
        template = 'django_tables2/bootstrap-responsive.html'
        fields = ('phone', 'last_used',)

@jieter
Copy link
Owner

jieter commented Dec 4, 2017

@ad-m Nice idea, could be very useful!

At first thought I'd suggest renaming the context argument to extra_context, because naming it context implies that that'll replace the context passed to the template. Did you consider this? What do you think?

@ad-m
Copy link
Contributor Author

ad-m commented Dec 4, 2017

@jieter , you are right. Fix'd. :shipit:

Copy link
Owner

@jieter jieter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change. I've added some other comments, will merge when those are addressed.

@@ -18,6 +18,7 @@ class TemplateColumn(Column):
Arguments:
template_code (str): template code to render
template_name (str): name of the template to render
extra_context (dict): template variables used to (optional)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems to be incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jieter , Can you suggest a full content of line? I do not know enough English to do it myself.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

extra_content (dict): optional extra template context.

@@ -26,24 +27,27 @@ class TemplateColumn(Column):
- *value* -- value from `record` that corresponds to the current column
- *default* -- appropriate default value to use as fallback
- *row_counter* -- The number of the row this cell is being rendered in.
- context variables pass as argument to column
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one too. Also: please use the same style as the other list items.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the style because the keys are listed before, and here the value of the context key is dynamic. No static keys of context, so no bold.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, makes sense! I would propose:

 - any context variables passed using the `extra_context` argument to `TemplateColumn`.


Example:

.. code-block:: python

class ExampleTable(tables.Table):
foo = tables.TemplateColumn('{{ record.bar }}')
# contents of `myapp/bar_column.html` is `{{ value }}`
bar = tables.TemplateColumn(template_name='myapp/name2_column.html')
# contents of `myapp/bar_column.html` is `{{label}}: {{ value }}`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add spaces around label to get consistent style with the other template variables in the example?

@ad-m
Copy link
Contributor Author

ad-m commented Dec 5, 2017

@jieter , changes introduced. Thank you for your comments and help.

@jieter
Copy link
Owner

jieter commented Dec 5, 2017

@ad-m thanks for the changes.

As the final piece of the puzzle, It would be nice if you could add a test case checking for the extra context to be present.

@jieter jieter merged commit 0bf3a9f into jieter:master Dec 5, 2017
@jieter
Copy link
Owner

jieter commented Dec 5, 2017

@ad-m thanks, merged!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants