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

Limited tag list #25

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

Limited tag list #25

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 31, 2018

I suggest to consent users to limit the tag printed on the screen, following the priority given by use frequency of tags.

zwitterio added 3 commits August 31, 2018 01:55
Add limit variable
Insert limit variable
Loop that limit the displaying of tags
@metbril metbril mentioned this pull request Aug 31, 2018
@metbril
Copy link

metbril commented Aug 31, 2018

Now that the number of settings is increasing, it might be wise to also add i18n through a languages.yaml.

@metbril
Copy link

metbril commented Aug 31, 2018

Looking at the twig, it actually also changes the class names and not only the logic, so this would not only change the number of displayed tags, but also how they are displayed.

For me, this PR should ONLY change the logic itself

<a class="{{ active }}" href="{{ base_url }}/{{ taxonomy }}{{ config.system.param_sep }}{{ tax }}">{{ tax }}</a>
{% set count = 0 %}
{% for tax,value in taxlist[taxonomy] if count < limit %}
{% set label_class = uri.param(taxonomy) == tax ? 'label-primary' : 'label-secondary' %}
Copy link

Choose a reason for hiding this comment

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

This also changes class names. Not only the logic.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't touch anything, just add a condition in the loop which refers to a variable in the taxonomy.yaml, because I noticed that the plugin already put the tags in order from the most used one to the less used ones.

Copy link

Choose a reason for hiding this comment

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

Well, the {% set active line has been removed, and you have introduced label-primary and label-secondary to the code. Your contribution is probably not originating from the plugin itself, but from a customized (themed?) twig.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, I forgot about my old modifications before posting the code.
I have to admit it: that's my first time posting something on github.
I'll amend the lines soon.

{% set active = uri.param(taxonomy) == tax? 'active' : '' %}
<a class="{{ active }}" href="{{ base_url }}/{{ taxonomy }}{{ config.system.param_sep }}{{ tax }}">{{ tax }}</a>
{% set count = 0 %}
{% for tax,value in taxlist[taxonomy] if count < limit %}
Copy link
Member

Choose a reason for hiding this comment

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

count < limit could cause issues if you have no limit value. need to also check if limit is set before this if is evaluated


{% if taxlist %}
<span class="tags">
{% for tax,value in taxlist[taxonomy] %}
{% set active = uri.param(taxonomy) == tax? 'active' : '' %}
Copy link
Member

Choose a reason for hiding this comment

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

Also does need this active state put back.

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.

2 participants