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

Phase out special-purpose field panel types #7684

Merged
merged 26 commits into from Mar 2, 2022

Conversation

gasman
Copy link
Collaborator

@gasman gasman commented Nov 6, 2021

This change makes it possible to use FieldPanel for all field types, making StreamFieldPanel, RichTextFieldPanel, ImageChooserPanel, DocumentChooserPanel and SnippetChooserPanel obsolete and PageChooserPanel almost obsolete (it's still provided as a convenience for passing the page_type or can_choose_root arguments, although even this can be replaced with a FieldPanel with an appropriate widget). This is achieved in part by a new extension to WagtailAdminModelForm, where apps can register their own form widgets to be used for a given model field type (so that, for example, a foreign key to Image always uses the AdminImageChooser widget, without having to specify it explicitly via the form class or EditHandler definition).

One thing that might need more consideration is the removal of docs for ImageChooserPanel and DocumentChooserPanel - these currently seem to be written as if they're the first introduction to the image and document models. If this is indeed the case (which wouldn't be great, as these are reference docs rather than howto docs...) we should probably add them back as a new first section on topics/images, and a new topics/documents page.

We might also want to note that if people have deliberately chosen to use FieldPanel instead of a chooser panel (or, for non-edit-handler-based forms, omitted specifying a chooser widget) because a plain dropdown works better for their use case, they'll now need to specify the Select widget explicitly.

@squash-labs
Copy link

squash-labs bot commented Nov 6, 2021

Manage this branch in Squash

Test this branch here: https://gasmancleanupremove-chooser-pa-b1ya1.squash.io

@lb-
Copy link
Member

lb- commented Nov 7, 2021

Is it possible to also do away with PageChooserPanel by making the can choose root and page type options pass through via some other kwarg?

Maybe attrs or similar?

Maybe we kill it and make limit_choices_to work like it does for other related things?

https://docs.djangoproject.com/en/3.2/ref/models/fields/#django.db.models.ForeignKey.limit_choices_to

@gasman
Copy link
Collaborator Author

gasman commented Nov 12, 2021

The only added functionality in PageChooserPanel now is to pass the page_type and can_choose_root arguments to the widget (as target_models and can_choose_root respectively), so a line like

PageChooserPanel('related_page', page_type=[EventPage, BlogPage])

can be rewritten as:

FieldPanel('related_page', widget=AdminPageChooser(target_models=[EventPage, BlogPage]))

The latter is more wordy and 'low-level', so I'm hesitant to push it as a recommended replacement for PageChooserPanel in that situation. I'd also be reluctant to hard-code any page-chooser-specific special cases in FieldPanel, which would rule out something like

FieldPanel('related_page', page_type=[EventPage, BlogPage])

and while there might be a middle ground like

FieldPanel('related_page', widget_args={'target_models': [EventPage, BlogPage]})

...I'm not sure if that's enough of an improvement to be worthwhile, or whether the page_type option is even widely used enough to worry about this in the first place. If we could embed this information into the model field definition via limit_choices_to, that would neatly sidestep the problem, but that's easier said than done - see #1250.

As a bit of additional context - I'm intending that the next step on this path will be to make 'foo' an alias of FieldPanel('foo') so that for the simplest setups (think snippets rather than pages), the panel definition will just be equivalent to writing a fields = ['first_name', 'last_name'] list on a ModelForm. Only when you want to customise the form layout would you start adding EditHandler-ish idioms:

panels = [
    MultiFieldPanel(['first_name', 'last_name']),  # actually we'd probably just rename it 'Row' or something...
    'photo',
]

...and hopefully along the way we'll introduce template components so that defining a wrapper like MultiFieldPanel is more a case of overriding a template and less deep Wagtail "bind an instance to a form" voodoo.

In that future scenario, then, I think PageChooserPanel in its current form will sit nicely as a 'power user' feature that you'll drop in if you want more control over how the form behaves - for a simple setup it'll be

content_panels = [
    'title', 'body', 'related_page'
]

but the 'advanced' version is:

content_panels = [
    'title', 'body',
    PageChooserPanel('related_page', page_types=[EventPage, BlogPage])
]

@gasman gasman force-pushed the cleanup/remove-chooser-panels branch from 5a054ad to 0e8f476 Compare February 8, 2022 01:06
@ababic
Copy link
Contributor

ababic commented Feb 9, 2022

@gasman We could adopt something similar to the pattern used by django.contrib.admin.Modeladmin.fieldsets

  • A tuple containing a dictionary is interpreted as a MultiFieldPanel, where the first item is interpreted as the panel heading
  • A tuple that contains only strings or FieldPanel instances is interpreted as a RowFieldPanel

Example:

content_panels = [
    (
        "Header",   # This is a multifieldpanel / queryset
        {
            "fields": [
                "title", 
                "banner_image",
                ("main_category", "secondary_categoy"),  # This is a row
                "publish_date".
            ]
        },  
    ),
    "body",
]

@gasman
Copy link
Collaborator Author

gasman commented Feb 10, 2022

@ababic I see the panels definition as being a 'pluggable' thing, where it would be a routine task for developers to create their own panel types if they so wish - so I'm reluctant to attach any syntactic special status to the built-in ones. With some well-thought-out API design I think we can make things readable enough that that isn't necessary:

from wagtail.panels import Fieldset, Row

...
    content_panels = [
        Fieldset("Header", [
            "title", 
            "banner_image",
            Row("main_category", "secondary_category"),
            "publish_date",
        ]),
        "body",
    ]

Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

Nice work! It's really great to see such a large reduction in complexity too

Only thing I wanted to pick up on was the title new forms doc. Also, do you think we should follow the how-to guide recommendations for this?

@@ -0,0 +1,58 @@
# Forms
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use a more descriptive title to make this easier to find? For example "Globally overriding form widgets"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've gone for "Using forms in admin views", as the instructions for overriding widgets are only one section of it. I think there's a risk with going overly specific on these titles, where people have to do extra thinking to figure out if it corresponds to the problem they're trying to solve - if they're coming to the docs with the question "how do I build a form with the Wagtail look and feel", they might not immediately make the connection to "overriding form widgets".

wagtail/utils/registry.py Show resolved Hide resolved
@gasman gasman force-pushed the cleanup/remove-chooser-panels branch 3 times, most recently from 09cbb1f to e84dd5e Compare February 18, 2022 15:11
gasman added 12 commits March 2, 2022 11:48
All built-in choosers use the standard field_panel_field.html template - it's probably been this way since Wagtail 1.0, when choosers got their own Widget classes rather than just being HTML decoration around a HiddenInput. As a result, the additional template context they pass (e.g. self.object_type_name) is redundant. If any third-party chooser panels exist that do need them (which is unlikely, if they just copy the existing ones), they should override render_as_field themselves to pass whatever context they need.
Removing the test is justified as this isn't a public-facing method; it's just a helper to get the object data onto the template, and the test_render_as_field test already confirms that's working correctly.
This has been part of django-modelcluster's ClusterForm since 3.1: wagtail/django-modelcluster#73
…et for a ForeignKey to a specific model

This means that pages, images, documents and snippets will use the approprate chooser widget automatically without having to specify it explicitly in the form definition or use a *ChooserPanel in the edit handler.
These are no longer necessary now that the correct widgets are selected at the form level.
…register their own form fields with WagtailAdminModelForm
…lowed by PageChooserPanel's page_type argument
@gasman gasman force-pushed the cleanup/remove-chooser-panels branch from e84dd5e to ea5530b Compare March 2, 2022 11:50
@gasman gasman force-pushed the cleanup/remove-chooser-panels branch from ea5530b to b937dad Compare March 2, 2022 12:03
Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks!

@gasman gasman merged commit 92cc026 into wagtail:main Mar 2, 2022
gasman added a commit that referenced this pull request Mar 2, 2022
zerolab added a commit to wagtail/wagtail-localize that referenced this pull request Mar 8, 2022
…2.17

as a result of the move to phase out special-purpose field panel types wagtail/wagtail#7684
zerolab added a commit to wagtail/wagtail-localize that referenced this pull request Mar 10, 2022
* Pass JSON to Page.with_content_json and tidy up JSON passed during convert alias
   Page revisions now use a JSONField as of wagtail/wagtail#8024

* Fix PageChooserPanel.target_models no longer available in Wagtail >= 2.17
   as a result of the move to phase out special-purpose field panel types wagtail/wagtail#7684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants