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

Design change related to issue #100 #101

Merged
merged 3 commits into from
May 25, 2023

Conversation

nicholsk18
Copy link
Contributor

I had to change the way data is being passed to the template for a cleaner and easier way to loop over that data.
Let me know if something needs to be refactored in inventoryitem_group.py file. Python is not my go-to language 😄

Copy link
Member

@matejv matejv left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I made a couple of suggestions to use status label in template.

Other than that the code seems fine and the end result looks great!

status_list = {
'status': tsc['status'],
'count': str(tsc['count']), # needs to be a string to render
'color': tsc['color']
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'color': tsc['color']
'color': tsc['color'],
'label': tsc['label']

Add status label (aka display)

<td class="d-flex">
{% for status in tsc.status_list %}
<a href="{% url 'plugins:netbox_inventory:asset_list' %}?inventoryitem_type_id={{ tsc.inventoryitem_type }}&status={{ status.status }}" class="w-100 me-2">
{% badge value=status.status|capfirst|add:' - '|add:status.count bg_color=status.color|add:' w-100' %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% badge value=status.status|capfirst|add:' - '|add:status.count bg_color=status.color|add:' w-100' %}
{% badge value=status.label|add:' - '|add:status.count bg_color=status.color|add:' w-100' %}

I suggest using status label here.

At least we use localized custom status values, so displaying them consistently throughout UI is desirable.

@nicholsk18
Copy link
Contributor Author

Thank you for the review, label does make more sense there. I also added overflow, for people who might have many badges, so it doesn't break their UI.

@matejv matejv merged commit 38789f4 into ArnesSI:master May 25, 2023
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