search

jieter / django-tables2

django-tables2 - An app for creating HTML tables
https://django-tables2.readthedocs.io/en/latest/
Other
1.86k stars 426 forks source link

Bugfix missing l10n load #919

Closed tvanekeris closed 10 months ago

tvanekeris commented 1 year ago

Added missing load tag "l10n" in all templates where "localize" filter is used.

jieter commented 1 year ago

@tvanekeris thanks for your contribution. Apparently, this is not commonly used, as I've never seen any reports of this bug.

Should we keep this functionality? Are you using it (with custom templates)?

tvanekeris commented 1 year ago

Hi @jieter, first of all thanks for the great project! I have built a custom template based on the templates from the project (because I am mostly not using bootstrap, but bulma). I have then seen that loading the l10n module is missing in the templates and wanted to play it back to the main project. So far I have no values in my tables that need the localization (most data is currently strings), but I will probably have in the future (e.g. for comma or point separator for decimals), so I think to keep the l10n in the templates is a good thing.

marksweb commented 1 year ago

You only need to load l10n if you're going to use the template tags/filters it provides which are localize and unlocalize (docs).

The templates are making use of these filters, so this seems a valid change 👍

If it's just localization of data like dates and numbers that you need, the USE_L10N setting should be all you need (docs).

tvanekeris commented 1 year ago

Hi, yes exactly. In every file where I added the {% load l10n %} the filter is used, e.g., in https://github.com/jieter/django-tables2/blob/966a41c25e242652bae982e71fc3cdd9ed4a66bf/django_tables2/templates/django_tables2/bootstrap.html#L30

It might work without the include (have not tested it) but according to the docs (and also hints from PyCharm IDE) the load should be added.

jieter commented 10 months ago

Thanks! What puzzles me is that this never failed for me in tests, or that we received any complaints about it. But I agree it looks to be necessary. I'll merge this.