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

Try and get folks to put more tags. #3350

Merged
merged 5 commits into from Dec 5, 2017
Merged

Try and get folks to put more tags. #3350

merged 5 commits into from Dec 5, 2017

Conversation

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 3, 2017

No description provided.

Copy link
Member

@humitos humitos left a comment

Take a look at the small changes that I am suggesting.

<h3>{% trans "Tags" %}</h3>
<p>
{% if project.tags.count %}
{% for tag in project.tags.all %}
<a href="{% url "projects_tag_detail" tag.slug %}">{{ tag.name }}</a>{% if forloop.last %}{% else %}, {% endif %}
{% empty %}
<span class="quiet">{% trans "No tags" %}</span>
Copy link
Member

@humitos humitos Dec 4, 2017

This condition won't be met since it has to be 0 on project.tags.count to have an empty list. I think you can remove the empty block.

{% for tag in project.tags.all %}
<a href="{% url "projects_tag_detail" tag.slug %}">{{ tag.name }}</a>{% if forloop.last %}{% else %}, {% endif %}
{% empty %}
<span class="quiet">{% trans "No tags" %}</span>
{% endfor %}
</p>
{% else %}
Project has no tags.
Copy link
Member

@humitos humitos Dec 4, 2017

trans is missing

{% else %}
Project has no tags.
{% if request.user|is_admin:project %}
Add some in your <a href="{% url 'projects_edit' project.slug %}">project settings</a>.
Copy link
Member

@humitos humitos Dec 4, 2017

trans is missing

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Dec 5, 2017

humitos
humitos approved these changes Dec 5, 2017
Copy link
Member

@humitos humitos left a comment

LGTM!

Just two comments to consider.

{% trans "Project has no tags." %}
{% if request.user|is_admin:project %}

{% url 'projects_edit' project.slug as edit_url %}
Copy link
Member

@humitos humitos Dec 5, 2017

I don't remember this exactly but, can we just use the url tag like this to create a local variable? don't we need the with tag for this?

Copy link
Member Author

@ericholscher ericholscher Dec 5, 2017

Yep, it works!

Copy link
Member Author

@ericholscher ericholscher Dec 5, 2017

<span class="quiet">{% trans "No tags" %}</span>
{% endfor %}

{% if project.tags.count %}
Copy link
Member

@humitos humitos Dec 5, 2017

I saw that we are using .count in some places where we don't need the number but we want to know the existence. So, maybe it's better to use .exists

Copy link
Member Author

@ericholscher ericholscher Dec 5, 2017

Hah yea, old habit!

@ericholscher ericholscher merged commit 6dfb237 into master Dec 5, 2017
2 checks passed
@stsewd stsewd deleted the better-tags branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants