-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #20264: Update plugin title rendering with default icon #20280
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
Fixes #20264: Update plugin title rendering with default icon #20280
Conversation
| return value | ||
| url = reverse('core:plugin', args=[record.config_name]) | ||
| return mark_safe(f"<a href='{url}'>{value}</a>") | ||
| order_by = ('-is_installed', '-is_certified', 'title_long') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the behavior of is_installed. Since it is a field that only renders a template fragment based on the values of is_local and is_loaded, and there is no is_installed concrete field on the Plugin (anymore), is it meaningful to use it as an ordering column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I guess this depends on #20267, with which it would make sense, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
You’re right: order_by="-is_installed" predates this PR and is currently broken because there’s no concrete field. That will be fixed by #20267.
This PR only corrects the bad reference by changing order_by="name" to order_by="title_long".
|
Thanks for calling this out! A few options to make this consistent:
Happy to implement whichever you prefer. |
|
I like the idea of option 1 I think. It's interesting that the current default "box+" icon suggests that you're supposed to click on it to install the plugin, but it's not clickable and does nothing; maybe the default "box" you're suggesting should take its place? I assume a catalog plugin can override the icon even if it's not installed? |
|
Thanks! One nuance to call out: The current “box+” icon doesn’t come from NetBox core; it’s delivered via the Plugin Catalog In this PR I can:
And yes, catalog plugins can still override the icon at any time (installed or not) by supplying their own |
Replaces inline plugin title HTML with a reusable template in `template_code.py`. Adds a default icon for plugins without custom icons and updates the table logic to use this template. Removes redundant logic from the `render_title_long` method to improve maintainability. Changes the `order_by` field in `plugins.py` from `name` to `title_long`. Fixes netbox-community#20264
bb38502 to
b24f8fb
Compare
|
Sure, this works for me. It should definitely spur us to make a better default icon for the catalog plugins, but now at least we have the flexibility for that. |

Fixes: #20264
Summary
template_code.py.title_longas aTemplateColumnusing the template; remove redundantrender_title_long().order_byfromname→title_long.Impact
icon_urlstill overrides the default.