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

Allow customising icons for snippets via SnippetViewSet.icon #10178

Closed
wants to merge 5 commits into from

Conversation

laymonage
Copy link
Member

@laymonage laymonage commented Mar 1, 2023

Rebase of #9804, fixes #4095, incorporates #10216 as part of #10206.

Unfortunately this does not fix #4040, as it requires overwriting the SnippetChooserBlock.Meta.icon attribute at runtime.

Hide whitespace when reviewing so that the indentation changes in the templates aren't shown.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
    • Please list which assistive technologies 3 you tested:
  • For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Define a subclass of SnippetViewSet, e.g. with the Person model in bakerydemo in wagtail_hooks.py:

from wagtail.snippets.models import register_snippet
from wagtail.snippets.views.snippets import SnippetViewSet


class PersonViewSet(SnippetViewSet):
    icon = "user"


register_snippet(Person, viewset=PersonViewSet)

Note: when merging, add Daniel Kirkham as co-author of this PR.

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

@squash-labs
Copy link

squash-labs bot commented Mar 1, 2023

Manage this branch in Squash

Test this branch here: https://laymonagecustom-snippets-icons-mdw1t.squash.io

@laymonage laymonage self-assigned this Mar 9, 2023
@laymonage laymonage added this to the 5.0 milestone Mar 9, 2023
@laymonage laymonage force-pushed the custom-snippets-icons branch 5 times, most recently from da5472f to e35670f Compare March 13, 2023 17:43
@laymonage laymonage changed the title WIP: Custom icons for snippets Custom icons for snippets Mar 13, 2023
@laymonage laymonage marked this pull request as ready for review March 13, 2023 17:43
@laymonage laymonage requested a review from gasman March 13, 2023 17:43
@laymonage laymonage changed the title Custom icons for snippets Allow setting SnippetViewSet.icon to be used across the admin views Mar 13, 2023
@laymonage laymonage changed the title Allow setting SnippetViewSet.icon to be used across the admin views Allow customising icons for snippets using SnippetViewSet.icon Mar 13, 2023
@@ -1,153 +1,146 @@
{% extends "wagtailadmin/base.html" %}
{% extends "wagtailadmin/generic/base.html" %}
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this to reduce boilerplate

{% load i18n wagtailadmin_tags %}

{% block titletag %}{% trans "Workflow progress" %}{% endblock %}
{% block main_content %}
<h2>{% icon object_icon classname="initial" %} {% latest_str object %}</h2>
Copy link
Member Author

@laymonage laymonage Mar 14, 2023

Choose a reason for hiding this comment

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

Previously this page doesn't have any header icon, but there's an icon beside the object's name:

Screenshot image

If we add an icon, I feel like it would look a bit weird to have the same icon for both, so I used list-ul as the header_icon and came up with object_icon for the object's icon specifically for this view:

Screenshot image

I chose the list-ul icon because that's what we currently use for the Workflow information dialog:

Screenshot image

The SnippetViewSet.icon will be passed as object_icon instead of header_icon.

@@ -106,6 +106,7 @@ def _register_snippet_immediately(model, viewset=None):
)

viewsets.register(admin_viewset)
model.snippet_viewset = admin_viewset
Copy link
Member Author

@laymonage laymonage Mar 14, 2023

Choose a reason for hiding this comment

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

A bit hesitant to add this, but the problem is that the bulk actions view instance is shared across all the snippet models, and it has its own registration mechanism. Thus, the icon needs to be resolved at request time rather than during the view class's instantiation. Adding this allows us to do self.model.snippet_viewset.icon in get_context_data() of the view.

I imagine this will be necessary as we add more config options to the ViewSet anyway. For example, if the base URL path is customisable through the viewset, then we will need a way to access the ViewSet for a given model. Another way is to create a registry that maps the model to the viewset, but I'm not sure it's worth it...

Or something simpler, we could change the SNIPPET_MODELS global list into a dictionary that maps each model to the viewset instance? People who rely on get_snippet_models() will still be able to iterate through it like a list of models, but we will be able to do something like get_snippet_models()[MyModel] and get the viewset. Still, it's not as handy compared to MyModel.snippet_viewset, though.

Happy to change the naming, e.g. admin_viewset or just viewset. (Though developers might want to have other viewsets that are not SnippetViewSet for the model as well?)

We can also do this in SnippetViewSet.on_register(), but we currently patch some other attributes to the model in this function, so I figured I put this here.

@laymonage laymonage mentioned this pull request Mar 14, 2023
8 tasks
@laymonage laymonage changed the title Allow customising icons for snippets using SnippetViewSet.icon Allow customising icons for snippets via SnippetViewSet.icon Mar 14, 2023
@laymonage laymonage force-pushed the custom-snippets-icons branch 2 times, most recently from c2a2e75 to 98dc57c Compare March 15, 2023 10:45
Copy link
Collaborator

@gasman gasman left a comment

Choose a reason for hiding this comment

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

Looks good to me and works well in my tests!

gasman added a commit that referenced this pull request Mar 22, 2023
@gasman
Copy link
Collaborator

gasman commented Mar 22, 2023

Merged in 5f154a5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to add custom icon to Snippet models
2 participants