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

Used slugify template tag for tabs not working with non-ASCII alphanumeric characters #237

Closed
EngFarisAlsmawi opened this issue Dec 23, 2022 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@EngFarisAlsmawi
Copy link

Hello,
Thank you for your amazing django package

When used slugify template tag for tabs not working with non-ASCII alphanumeric characters

<button type="button" id="tablink-{{ tab_name|slugify }}" class="tabbed-changeform-tablink {{ forloop.counter0|default:'active' }}" onclick="AdminInterface.tabbedChangeForm.openTab(event, '{{ tab_name|slugify }}')">

instated you can used forloop counter

used tab with loop of counter for fieldset

<button type="button" id="tablink-tab{{  forloop.counter   }}" class="tabbed-changeform-tablink {{ forloop.counter0|default:'active' }}" onclick="AdminInterface.tabbedChangeForm.openTab(event, 'tab{{forloop.counter }}')">
                    {{ tab_name|capfirst }}
                </button>

Used** tab0 for fieldsets without tabs

used itab with loop counter for inlines formset

<button type="button" id="tablink-itab{{ forloop.counter }}" class="tabbed-changeform-tablink" onclick="AdminInterface.tabbedChangeForm.openTab(event, 'itab{{ forloop.counter }}')">
                    {{ tab_name|capfirst }}
                </button>

this is full patch code:

  <div id="tabbed-changeform-tabs" class="tabbed-changeform-tabs">

            {% if show_fieldsets_as_tabs %}
                {% for fieldset in adminform %}

                {% with fieldset.name|default_if_none:opts.verbose_name as tab_name %}
                <button type="button" id="tablink-tab{{  forloop.counter   }}" class="tabbed-changeform-tablink {{ forloop.counter0|default:'active' }}" onclick="AdminInterface.tabbedChangeForm.openTab(event, 'tab{{forloop.counter }}')">
                    {{ tab_name|capfirst }}
                </button>
                {% endwith %}

                {% endfor %}
            {% else %}
                {% with opts.verbose_name as tab_name %}
                <button type="button" id="tablink-tab0" class="tabbed-changeform-tablink active" onclick="AdminInterface.tabbedChangeForm.openTab(event, 'tab0')">
                    {{ tab_name|capfirst }}
                </button>
                {% endwith %}
            {% endif %}

            {% if show_inlines_as_tabs %}
                {% for inline_admin_formset in inline_admin_formsets %}
                {% with inline_admin_formset.opts.verbose_name_plural as tab_name %}
                <button type="button" id="tablink-itab{{ forloop.counter }}" class="tabbed-changeform-tablink" onclick="AdminInterface.tabbedChangeForm.openTab(event, 'itab{{ forloop.counter }}')">
                    {{ tab_name|capfirst }}
                </button>
                {% endwith %}
                {% endfor %}
            {% endif %}

            <span class="tabbed-changeform-tabs-remaining-space"></span>

        </div>

        {% if show_fieldsets_as_tabs %}
            {% for fieldset in adminform %}
            {% with fieldset.name|default_if_none:opts.verbose_name as tab_name %}
            <div id="tabcontent-tab{{ forloop.counter }}" class="tabbed-changeform-tabcontent {{ forloop.counter0|default:'active' }}">
                {% include "admin/includes/headerless_fieldset.html" %}
            </div>
            {% endwith %}
            {% endfor %}
        {% else %}
            {% with opts.verbose_name as tab_name %}
            <div id="tabcontent-tab0" class="tabbed-changeform-tabcontent active">
            {% for fieldset in adminform %}
                {% include "admin/includes/fieldset.html" %}
            {% endfor %}
            </div>
            {% endwith %}
        {% endif %}

        {% for inline_admin_formset in inline_admin_formsets  %}
        {% with inline_admin_formset.opts.verbose_name_plural as tab_name %}
        <div id="tabcontent-itab{{ forloop.counter }}" class="tabbed-changeform-tabcontent">
            {% get_admin_interface_inline_template inline_admin_formset.opts.template as inline_template %}
            {% include inline_template %}
        </div>
        {% endwith %}
        {% endfor %}
@fabiocaccamo fabiocaccamo self-assigned this Dec 23, 2022
@fabiocaccamo fabiocaccamo added the bug Something isn't working label Dec 23, 2022
@fabiocaccamo fabiocaccamo moved this to Todo in Open Source Dec 23, 2022
@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Dec 23, 2022

Hi @EngFarisAlsmawi, thank you for reporting this problem.

The slugified tab name is used for displaying readable urls (and changing the order of tabs doesn't impact direct links), for this reason I would not fix it using tab numbers, but I would fix this by using python-slugify instead of the django's slugify method.

@EngFarisAlsmawi
Copy link
Author

EngFarisAlsmawi commented Dec 24, 2022

Hi @fabiocaccamo , thank you for quick response.
Of course , python-slugify is a helpful package if you would like to displaying readable urls.

Although I think you'd be careful of this:
{% with fieldset.name|default_if_none:opts.verbose_name as tab_name %}

I noticed that you used the model verbose name as an alternative to the fieldset that do not have a name,
But what if your admin model owns more than fieldset without a name.

@merwok
Copy link
Contributor

merwok commented Feb 7, 2023

Personnally I would find it OK to get generated IDs like fs1, fs2, etc for fieldsets.

@EngFarisAlsmawi
Copy link
Author

In addition, currently I am using inline_admin_formset.formset.prefix for inline_admin_formsets.

@rikengct
Copy link

rikengct commented Feb 21, 2023

I reported this bug separately by mistake. but, i don't really understand the responses here.
do you (@fabiocaccamo) think this problem is important enough to fix it in the next release?
or is there a solution here in the comments that I missed because I did not understand?

@fabiocaccamo
Copy link
Owner

@rikengct yes it will be fixed in the next release.

@offbrok
Copy link

offbrok commented Apr 10, 2023

I can offer a temporary solution:
https://github.com/tony/django-slugify-processor

@merwok
Copy link
Contributor

merwok commented Apr 10, 2023

TBH that sounds a little heavy to me, and could conflict with usage of the same package by a project.

@fabiocaccamo
Copy link
Owner

@offbrok thank you for the suggestion, but it seems a little bit overkill for this issue.

The problem to solve is very simple and python-slugify solves the problem perfectly.

@offbrok
Copy link

offbrok commented Apr 11, 2023

@fabiocaccamo Perhaps my solution is more complicated, but it will help solve the problem for those who need non-standard functionality slugify. For example, in addition to slugify you need to translate to another language. I suggested this to people who will look for a solution and find it here.

@fabiocaccamo
Copy link
Owner

@EngFarisAlsmawi @rikengct @offbrok @merwok fixed in 0.25.0 version.

@github-project-automation github-project-automation bot moved this from Todo to Done in Open Source Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

5 participants