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

CHORE: Refactor RecordChooser to use built-in Wagtail components #865

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

ababic
Copy link
Contributor

@ababic ababic commented May 4, 2023

About these changes

We currently use wagtail-generic-chooser, which has since had parts of it re-implemented in Wagtail. This PR refactors our custom RecordChooser views and widget to use the built-in stuff, allowing us to ditch wagtail-generic-chooser as a dependency.

How to check these changes

Head to the image library and use try interacting with the 'record' field there.

Before assigning to reviewer, please make sure you have

  • Checked things thoroughly before handing over to reviewer.
  • Checked PR title starts with ticket number as per project conventions to help us keep track of changes.
  • Ensured that PR includes only commits relevant to the ticket.
  • Waited for all CI jobs to pass before requesting a review.
  • Added/updated tests and documentation where relevant.

Merging PR guidance

Follow docs\developer-guide\contributing.md

Deployment guidance

Follow docs\infra\environments.md

@ababic ababic marked this pull request as draft May 4, 2023 10:41
@ababic ababic force-pushed the chore/refactor-recordchooser branch from cc73381 to 2aa791c Compare May 4, 2023 10:44
@ababic ababic force-pushed the chore/refactor-recordchooser branch from 2aa791c to fac0059 Compare May 18, 2023 13:12
@LeanneEmans
Copy link

@jamesbiggs and @ahosgood - I wonder if this was discussed yesterday in the Dev Stand up. Any idea what the future of this PR is please?

@ahosgood
Copy link
Member

@LeanneEmans - Got to be honest here... I have no context for this but I completely understand the intention. Not quite sure I'd know what to expect to see in the record field or how to know if it works. 🤔

@jamesbiggs
Copy link
Collaborator

@LeanneEmans This PR isn't massively necessary. ATM we're using a third-party package called wagtail-generic-chooser which is fine to use, but Wagtail has kind of "absorbed" the package and uses elements from it in Wagtail itself. I think the idea here was to move to use the built-in Wagtail choosers, as we're then not relying on a third-party package. I can see Andy's done a lot of work on this, but this work is not urgent nor necessary. I think it was more of just a nice-to-have, but quite a major one in terms of the changes it'd bring in. I'd like to leave it here in draft in case any of us want to pick it up in the future.

@ahosgood ahosgood added the understanding Work relating to the Understanding team label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
understanding Work relating to the Understanding team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants