Refactor generic view subclasses to better reuse the generic templates and breadcrumbs #10884

laymonage · 2023-09-12T14:43:38Z

Built on top of #10880.

This adds consistent breadcrumbs in index/create/edit views to:

Workflows and tasks
Users
Groups
Sites
Locales
Collections

I also added a missing breadcrumb item on the snippets model index view (/admin/snippets/). In #10880 before it was reviewed, the view doesn't have any breadcrumbs. After moving the Home item to the WagtailAdminTemplateMixin instead of the breadcrumbs tag, this view now has the Home item but missing the non-linked "Snippets" item. I've fixed it in this PR and added tests for it (as well as tests for the snippets views breadcrumbs).

TODO, outside of this PR as the views code needs to be refactored to extend the generic CBVs or at least the WagtailAdminTemplateMixin, as they're currently either FBVs or custom CBVs:

Redirects (Add Go Back button in Redirects (breadcrumbs) #8037)
Search promotions
Images
Documents
Forms

When reviewing, please hide whitespace.

Please check the following:

Do the tests still pass?¹
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?

Development Testing ↩

squash-labs · 2023-09-12T14:43:43Z

Manage this branch in Squash

Test this branch here: https://laymonagealmost-universal-brea-dzukh.squash.io

lb- · 2023-09-15T11:08:10Z

@laymonage just a note that I created an umbrella/epic issue for tracking some of the conversion to class based views a while ago. It has some notes about adoption and breaking down of tasks.

Not sure if it's still a useful way to manage this but in case you have not seen it I thought I would mention it here - #8365

laymonage · 2023-09-15T15:42:59Z

wagtail/admin/templates/wagtailadmin/workflows/create.html

+{% block main_header %}
+    {% include "wagtailadmin/shared/header.html" with title=page_title subtitle=page_subtitle icon=header_icon merged=1 only %}
+{% endblock %}
+


The override is only necessary to pass merged=1 to the header template. Alternatively we can set it from the view, but I don't think it's worth adding more code to the view for something that would only change one CSS class which potentially would change in the future.

laymonage · 2023-09-15T15:43:46Z

wagtail/admin/templates/wagtailadmin/workflows/create.html

+    {% block header %}
+        {{ block.super }}
+    {% endblock %}


This is necessary because we're overriding content instead of main_content, and the reason for that is because we don't want to use the nice-padding wrapper div in this template.

laymonage · 2023-09-15T15:44:27Z

wagtail/admin/templates/wagtailadmin/workflows/create_task.html

+    {% block header %}
+        {{ block.super }}
+    {% endblock %}


Same reason, no nice-padding in this template. If nice-padding is used, this isn't necessary and we can override the main_content block instead

laymonage · 2023-09-15T15:46:09Z

wagtail/admin/templates/wagtailadmin/workflows/task_index.html

@@ -12,8 +12,10 @@
        </a>
        <a href="{% url view.add_url_name %}" class="button bicolor button--icon">{% icon name="plus" wrapped=1 %}{{ view.add_item_label }}</a>
    {% endfragment %}
-    {% include "wagtailadmin/shared/header.html" with title=view.page_title icon=view.header_icon extra_actions=workflow_actions %}
+    {% include "wagtailadmin/shared/header.html" with title=page_title icon=header_icon extra_actions=workflow_actions %}


Just changing from view.page_title and view.header_icon to the page_title and header_icon directly for consistency, because the former uses the variables while the latter is resolved from the corresponding get_ methods and are available via WagtailAdminTemplateMixin.get_context_data()

laymonage · 2023-09-15T15:47:42Z

wagtail/users/templates/wagtailusers/groups/edit.html

    {% trans "View users in this group" as users_str %}
    {% url 'wagtailusers_groups:users' group.id as group_users_url %}
-    {% include "wagtailadmin/shared/header.html" with title=editing_str action_icon="user" action_url=group_users_url action_text=users_str subtitle=group.name icon="group" %}
+    {% include "wagtailadmin/shared/header.html" with title=page_title action_icon="user" action_url=group_users_url action_text=users_str subtitle=page_subtitle icon="group" %}


Reusing the page_title and page_subtitle context variables instead of redefining them ad-hoc in the template. The resolved values are the same though

laymonage · 2023-09-15T15:48:20Z

wagtail/users/templates/wagtailusers/users/create.html

-{% block extra_css %}
-    {{ block.super }}
-    {{ form.media.css }}
-{% endblock %}


This is already covered in wagtailadmin/generic/form.html

laymonage · 2023-09-15T15:49:22Z

wagtail/users/templates/wagtailusers/users/index.html

-    {% trans "Users" as users_str %}
-    {% trans "Add a user" as add_a_user_str %}
-
-    {% url "wagtailusers_users:add" as add_link %}
-    {% include "wagtailadmin/shared/header.html" with subtitle=group.name title=users_str action_url=add_link action_text=add_a_user_str icon="user" search_url="wagtailusers_users:index" search_results_url=index_results_url %}


Rely on the view code to define the variables for the header and just reuse the includes from the generic template

laymonage · 2023-09-15T15:51:24Z

wagtail/users/views/users.py

-    def get_page_title(self):
-        return _("Editing %(object)s") % {"object": self.object.get_username()}
-
-    def get_page_subtitle(self):
-        return ""


Reusing the format from the generic EditView instead for consistency, where the title is Editing and the subtitle is str(self.object). By default str() returns the username, but this means now the title will reflect whatever is returned by __str__ instead of forcing it to be the username.

laymonage · 2023-09-15T16:00:57Z

wagtail/users/views/groups.py

+    def get_page_subtitle(self):
+        return ""


I'm retaining the behaviour of showing "Add group" as the title (without subtitle) instead of the generic view's "New" title + capfirst({model_name}) subtitle, though we probably should change this for consistency with the generic view.

One downside I see with this is that in other languages, the correct form might be the other way around (model_name + "New").

However, the assumption of action + model_name has been the case with the generic views so I suppose it's alright... I think ideally we should not have a design where page_title and page_subtitle is shown side-by-side to form a phrase, e.g. "Editing Muddy Waters", as this is the root cause of the aforementioned issue with other languages

I think the capfirst stuff has caused us translation issues in the past, probably best to avoid unless there is a critical reason to add it.

laymonage · 2023-09-15T16:02:56Z

Thanks @lb- yep I'm aware of that issue, I think that is still handy to track the views needed refactoring. Might be a good candidate for Outreachy/GSoC participants as well, though ideally they need to have a good level of understanding in class-based views and Django templates.

lb-

@laymonage this looks great to me.

I am happy to approve this and think it's another epic win for the next release, both in UI enhancements and also a more consistent approach to our admin templates.

Is it worth calling out the block main_content/content structure in the upgrade considerations? I think existing custom template overrides would likely work OK but I know we document overriding template paths as a feature and it may be good to add this to the upgrade considerations.

We have not historically documented all template structure changes like this but I know we did do a brief overview of changes for some of the Breadcrumb /header templates in previous releases.

No blockers for this to be merged though, release notes addition can be added when you merge or after.

If it's ok, I will let you merge, not sure if you wanted to re-work some of the commits/squash a bit.

laymonage · 2023-09-18T16:01:55Z

Thibaud and I discussed this again with our designers and while we are not against the idea of showing the breadcrumbs everywhere in the admin, there are nuances around the <h1> heading and the actions in the slim header that ideally we'd like to solve before showing the breadcrumbs in more places.

At the moment, the edit page for the Person snippet on bakerydemo for example, looks like the following:

The text "Muddy Waters" shows up three times in different parts, close to each other.

We explored the idea of merging the <h1> with the breadcrumbs, kind of like how the page listing view looks, but that would increase the sticky header's size, which would consume a good amount of screen real estate even after you scrolled down. In short, there are still some UI design decisions to be made on the breadcrumbs.

The breadcrumbs added in #10880 were not really intentional, as the main reason for that PR was so that we have a place to put the history icon link when we add HistoryView to ModelViewSet later. The breadcrumbs changes were made to the generic views, so some views that extend them have the breadcrumbs as a side-effect.

Given the original motivation of #10880 and the design decisions that are yet to be made, it's likely we won't proceed with this PR just yet, and instead we'll only enable the breadcrumbs on ModelViewSets that are specifically created by developers as a result of #10740.

lb- · 2023-09-18T20:57:38Z

Ok. Sorry @laymonage - I had assumed that direction has been given already to have the Breadcrumbs even when we have page headers so I didn't think to ask about it.

Is it possible to have a version of this PR that just does the template clean up? I think that part is still quite valuable, even if the breadcrumbs are not used universally.

laymonage · 2023-09-19T08:32:21Z

@lb- No worries, I was still kind of uncertain when I worked on this and we didn't find the time to have a proper discussion until yesterday.

Is it possible to have a version of this PR that just does the template clean up? I think that part is still quite valuable, even if the breadcrumbs are not used universally.

I think it is possible, and given the example in #10915 I might even be able to rebase that PR on top of this as-is. I'll see what I can do and report back.

…crumbs to appear

This allows us to reuse the header parts, including breadcrumbs.

This allows us to reuse the breadcrumbs.

…late

…ty template

…use the slim header

…templates

…m_disable template

…delViewSet breadcrumbs tests

thibaudcolas

Just a quick note that Sage and I discussed this refactoring and I’m happy with the code changes. Haven’t actually tested the proposed changes myself.

thibaudcolas · 2023-10-03T14:02:48Z

wagtail/test/utils/template_tests.py

+    base_breadcrumb_items = [{"label": "Home", "url": "/admin/"}]
+
+    def assertBreadcrumbsItemsRendered(
+        self: Union[WagtailTestUtils, SimpleTestCase],


Hmm, annoying that this doesn’t convey the need for the WagtailTestUtils instance to also inherit from TestCase. I wonder if there’s a better way to annotate this.

Yeah unfortunately I couldn't find a better way to do it. The WagtailTestUtils class is basically a mixin, and it doesn't assume which TestCase base class you're going to use it with. For example, you can use it with a SimpleTestCase or the more featured TestCase depending on your needs, but we can't enforce this unless we make WagtailTestUtils extend from that (at which point it might break existing tests because they now inherit the TestCase twice).

I just opted for the most basic class needed (SimpleTestCase) and used Union with WagtailTestUtils to at least get VSCode to understand where the methods come from.

Alternatively you could define a class just for the sake of typing, e.g.

class WagtailTestUtilsTestCase(WagtailTestUtils, SimpleTestCase): ...

and use it to annotate self, but I don't really like doing this because it would actually define the class (unlike a TypeScript interface that doesn't produce runtime code). VSCode's type shims tend to use this approach with .pyi files.

laymonage

Thanks all. We had another design review session yesterday, and made some decisions:

The final item in the breadcrumbs should be a link for consistency and to follow the examples in https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/. If it's not a link, the last item has a different colour which looks inconsistent.
- The only place where this isn't the case (aka trailing_breadcrumb_title) is currently the page create view. I'll try to make it a link as well in Refactor page_breadcrumbs tag to use shared breadcrumbs.html template #10876, though this is currently low-priority for 5.2.
We'll disable the breadcrumbs in all views other than pages, snippets, and custom ModelViewSets for now. This is because we still need further rework on the designs before introducing the breadcrumbs more widely, especially with the upcoming Universal Listings work.
- This will be done in Disable breadcrumbs outside of pages, SnippetViewSet, and custom ModelViewSet #10915.

I've rebased this and added another commit 84a4835 to make the final item in the breadcrumbs a link.

Merging this now, further cleanups will be done in separate PRs as I mentioned above.

laymonage self-assigned this Sep 12, 2023

laymonage added type:Enhancement type:Cleanup/Optimisation component:Design system Including the pattern library (Storybook) status:Needs Tests labels Sep 12, 2023

laymonage added this to the 5.2 milestone Sep 12, 2023

laymonage force-pushed the almost-universal-breadcrumbs branch 2 times, most recently from fd814a3 to b8bb58b Compare September 13, 2023 17:00

laymonage marked this pull request as ready for review September 13, 2023 17:01

This was referenced Sep 13, 2023

Add Go Back button in Redirects (breadcrumbs) #8037

Open

Add breadcrumbs to generic model views #10880

Merged

laymonage force-pushed the almost-universal-breadcrumbs branch 3 times, most recently from fe212f4 to 60d9a5a Compare September 14, 2023 15:28

laymonage added status:Needs Review and removed status:Needs Tests labels Sep 14, 2023

laymonage force-pushed the almost-universal-breadcrumbs branch 2 times, most recently from df5f1f8 to bd3f0a6 Compare September 15, 2023 15:39

laymonage requested a review from lb- September 15, 2023 15:39

laymonage commented Sep 15, 2023

View reviewed changes

lb- approved these changes Sep 18, 2023

View reviewed changes

thibaudcolas removed the status:Needs Review label Oct 3, 2023

laymonage added 2 commits October 3, 2023 15:01

Add missing model attribute in collections create view to allow bread…

7e7fc56

…crumbs to appear

Extend generic base template for groups create template

0709c31

This allows us to reuse the header parts, including breadcrumbs.

laymonage added 22 commits October 3, 2023 15:01

Extend generic base template for groups edit template

5272161

This allows us to reuse the breadcrumbs.

Extend generic confirm_delete template for groups confirm_delete temp…

5cf38dd

…late

Extend generic confirm_delete template for locale confirm_delete temp…

0ac0402

…late

Extend generic confirm_delete template for collections delete_not_emp…

c2b8a71

…ty template

Reuse header definition from the generic index view for users index view

f7b597b

Extend generic form template for users edit template

f90f900

Extend generic confirm_delete template for users confirm_delete template

466d2b6

Extend generic form template for users create template

b291c9b

Override specific blocks in workflows and tasks index templates to re…

fc33e4a

…use the slim header

Extend generic base template for workflows and tasks create and edit …

21b7383

…templates

Extend generic confirm_delete template for workflows and tasks confir…

8322dd0

…m_disable template

Use base Task verbose name for task index breadcrumb item

815bf01

Fix failing tests due to str(response.content) when parsing HTML

b487ea6

Extract AdminTemplateTestUtils.assertBreadcrumbsItemsRendered from Mo…

5349252

…delViewSet breadcrumbs tests

Add tests for rendering breadcrumbs on user and group views

76ad1bf

Add tests for rendering breadcrumbs on collections views

4bf2a93

Add tests for rendering breadcrumbs on sites views

e090d74

Add tests for rendering breadcrumbs on locales views

5bc3fbf

Add tests for rendering breadcrumbs on workflows and tasks views

8dc376c

Add tests for rendering breadcrumbs on snippets views

8fa39ea

Add missing capfirst() in generic usage view breadcrumbs item

fb5d22a

Add breadcrumbs to the snippets model index view

3b7aa24

laymonage force-pushed the almost-universal-breadcrumbs branch from bd3f0a6 to 3b7aa24 Compare October 3, 2023 14:08

thibaudcolas reviewed Oct 3, 2023

View reviewed changes

laymonage added 2 commits October 3, 2023 15:46

Make the last item in the breadcrumbs a link to itself

84a4835

Release notes for wagtail#10884

87f1310

laymonage commented Oct 3, 2023

View reviewed changes

laymonage merged commit 87f1310 into wagtail:main Oct 3, 2023
17 checks passed

laymonage mentioned this pull request Oct 5, 2023

Disable breadcrumbs outside of pages, SnippetViewSet, and custom ModelViewSet #10915

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor generic view subclasses to better reuse the generic templates and breadcrumbs #10884

Refactor generic view subclasses to better reuse the generic templates and breadcrumbs #10884

laymonage commented Sep 12, 2023 •

edited

squash-labs bot commented Sep 12, 2023

lb- commented Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023

laymonage Sep 15, 2023 •

edited

lb- Sep 15, 2023

laymonage commented Sep 15, 2023

lb- left a comment

laymonage commented Sep 18, 2023 •

edited

lb- commented Sep 18, 2023

laymonage commented Sep 19, 2023

thibaudcolas left a comment

thibaudcolas Oct 3, 2023

laymonage Oct 3, 2023

laymonage left a comment

Refactor generic view subclasses to better reuse the generic templates and breadcrumbs #10884

Refactor generic view subclasses to better reuse the generic templates and breadcrumbs #10884

Conversation

laymonage commented Sep 12, 2023 • edited

Footnotes

squash-labs bot commented Sep 12, 2023

Manage this branch in Squash

lb- commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laymonage Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laymonage commented Sep 15, 2023

lb- left a comment

Choose a reason for hiding this comment

laymonage commented Sep 18, 2023 • edited

lb- commented Sep 18, 2023

laymonage commented Sep 19, 2023

thibaudcolas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laymonage left a comment

Choose a reason for hiding this comment

laymonage commented Sep 12, 2023 •

edited

laymonage Sep 15, 2023 •

edited

laymonage commented Sep 18, 2023 •

edited