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

Feature/adding template filter for switch flag sample usable for if statement conditions #532

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MatthewEthanTam
Copy link

This PR is addressing #510, as the previous PR was out of sync

@MatthewEthanTam
Copy link
Author

@ulgens Please see new PR rebased to master and removed updates to .po files.

return waffle_switch_is_active(switch_name)


@register.simple_tag # type: ignore[misc]
Copy link
Contributor

@dancergraham dancergraham Nov 18, 2024

Choose a reason for hiding this comment

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

question: should this be a register.filter like the switch and sample above and below ? context: I don't use django templates so apologies if I'm missing something obvious

Copy link
Author

Choose a reason for hiding this comment

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

Good question @dancergraham , so since there are more than 2 arguments with differing types for this function, we cannot use a template filter for it, therefore, we use a simple tag and set the variable in the django template.

{% flag_is_active "flag" request True as is_flag_active %}
{% if is_flag_active %}
  flag_is_active on
{% else %}
  flag_is_active off
{% endif %}

Copy link
Author

Choose a reason for hiding this comment

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

Sadly it is not as clean of an implementation, and there are forums about this exact issue dating back to 2007 (https://code.djangoproject.com/ticket/1199)

@ulgens
Copy link
Member

ulgens commented Nov 20, 2024

@MatthewEthanTam I checked the PR and issue several times and I still don't understand the case here. I'll check it again when I have better availability. Sorry for the delay.

@MatthewEthanTam
Copy link
Author

MatthewEthanTam commented Nov 20, 2024

@ulgens Thank you for your time and no worries about the delay, I an happy to contribute!

There are two cases.

First, the case explained in the issue is where code is duplicated when a Switch and a context variable are needed to display text/ elements in an HTML block. The implementation will allow for cleaner HTML code and reduced lines of code.

Before implementation:

{% switch 'switch'%}
  {% if context_variable %}
    modified message
  {% else %}
    default message
  {% endif %}
{% else %}
  default message
{%endswitch%}

After implementation:

{% if 'switch'|switch_is_active and context_variable%}
  modified message
{% else %}
  default message
{% endif %}

Second, HTML linters/formatters such as DJLint do not recognize the {% switch 'switch_name' %} template tag, but do recognize {% if switch_is_active|'switch_name' %}, allowing for a better linting & formatting experience.

@MatthewEthanTam MatthewEthanTam force-pushed the feature/adding-template-filter-for-switch-flag-sample-usable-for-if-statement-conditions branch from 41eedd9 to bcab6f7 Compare November 20, 2024 13:14
@MatthewEthanTam MatthewEthanTam force-pushed the feature/adding-template-filter-for-switch-flag-sample-usable-for-if-statement-conditions branch from bcab6f7 to 63f7179 Compare December 3, 2024 07:32
@MatthewEthanTam MatthewEthanTam force-pushed the feature/adding-template-filter-for-switch-flag-sample-usable-for-if-statement-conditions branch from 63f7179 to 30e57d3 Compare December 19, 2024 11:52
Copy link
Collaborator

@clintonb clintonb left a comment

Choose a reason for hiding this comment

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

Can you add documentation, please?

@dancergraham
Copy link
Contributor

@MatthewEthanTam do you need any help putting together the documentation?

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 this pull request may close these issues.

4 participants