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

Support for Wagtail v5.0 #34

Closed
wants to merge 17 commits into from
Closed

Support for Wagtail v5.0 #34

wants to merge 17 commits into from

Conversation

nickmoreton
Copy link
Contributor

@nickmoreton nickmoreton commented May 31, 2023

This PR updates the package for Wagtail v5.0 compatibility.

It also includes the following PR's and they could be closed if this is accepted:

Some of the tests are no longer relevant. They have been updated but the original assertions left in place as comments and may be useful for the maintainer to review and update.

The UI is no longer rendered as the screen shots show and further work would be required to fix that but the select boxes work as expected.

@nickmoreton nickmoreton marked this pull request as ready for review May 31, 2023 14:52
This was referenced May 31, 2023
Comment on lines +22 to +27
# choose_one_text = _("Choose a page")
# display_title_key = "display_title"
# chooser_modal_url_name = "wagtailadmin_choose_page"
# icon = "doc-empty-inverse"
# classname = "page-chooser"

Copy link

Choose a reason for hiding this comment

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

I think we should remove this if not being used.



class InstanceSelectorWidget(AdminChooser):
class InstanceSelectorWidget(AdminPageChooser):
Copy link

Choose a reason for hiding this comment

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

Hi Nick, upgrading M+ (one of the websites using this plugin) to Wagtail version 4.2, I used this branch to update the wagtail-instance-selector package.

The M+ team raised some problems wherein:

  • the search function within the instance selector modal was missing
  • some columns are not visible
  • there is no "Add" button

Before the upgrade, the instance selector modal looked like this:
image

After the upgrade, the modal looked like this:
image

We ended up using this branch to retain the old functionalities of the modal:
https://github.com/H1ako/wagtail-instance-selector

image

It seems that this user forked this project after learning that this project wasn't maintained anymore (see discussion):
#28 (comment)

I just realized that the difference is that their InstanceSelectorWidget retained usage of the AdminChooser, which was deprecated on Wagtail 4.0

With that, if we wanted to retain the old behaviour (being, able to search, add items, etc.), we might want to instead inherit from WidgetWithScript and widgets.Input much like what the deprecated AdminChooser did:
https://github.com/wagtail/wagtail/blob/stable/4.2.x/wagtail/admin/widgets/chooser.py#L23

What do you think?

Copy link

Choose a reason for hiding this comment

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

If we were to consider the above, since we stopped inheriting from BaseChooser or AdminPageChooser, we'll have to add the following class variables:

# when looping over form fields, this one should appear in visible_fields, not hidden_fields
# despite the underlying input being type="hidden"
input_type = "hidden"
is_hidden = False

@@ -28,6 +19,12 @@ def __init__(self, model, **kwargs):
self.choose_another_text = _("Choose another %s") % model_name
self.link_to_chosen_text = _("Edit this %s") % model_name

Copy link

Choose a reason for hiding this comment

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

If we were to Inherit from WidgetWithScript and widgets.Input, we'd need to include the following on init

self.clear_choice_text = _("Clear choice")
self.show_edit_link = True
self.show_clear_link = True

model = None
field_name = None

def widget_overrides(self):
return {self.field_name: InstanceSelectorWidget(model=self.target_model)}

Copy link

Choose a reason for hiding this comment

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

Since this was deleted, and we're using get_form_options for Wagtail 3.0 and up, we might want to override get_form_options to use the InstanceSelectorWidget:
https://github.com/H1ako/wagtail-instance-selector/blob/master/instance_selector/edit_handlers.py#L22

def get_form_options(self):
    opts = super().get_form_options()
    opts["widgets"] = {self.field_name: InstanceSelectorWidget(model=self.target_model)}
    return opts

@katdom13
Copy link

katdom13 commented Jun 5, 2023

Hi @nickmoreton , I created a PR on your branch to apply the suggested changes from my comments:
https://github.com/nickmoreton/wagtail-instance-selector/pull/4

I reckon I'll just merge my changes in this branch to avoid further confusion due to multiple PRs on the parent project involving the 5.0 upgrade.

Thanks!

@katdom13 katdom13 mentioned this pull request Jun 14, 2023
@katdom13
Copy link

HI @nickmoreton ,
This PR can be closed in favor of: #35

@nickmoreton
Copy link
Contributor Author

Closing as this PR is no longer required.

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.

3 participants