-
Notifications
You must be signed in to change notification settings - Fork 92
Implemented and added media chooser into the edit translation workflow for audio and video files #847
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
base: main
Are you sure you want to change the base?
Implemented and added media chooser into the edit translation workflow for audio and video files #847
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for starting on this @catilgan-nextension
Had a quick look and left a few notes and suggestions.
This will need a few tests too.
tox.ini
Outdated
@@ -82,6 +82,7 @@ basepython = python3.12 | |||
deps = | |||
wagtail>=5.2,<6.3 | |||
Django>=4.2,<5.2 | |||
wagtailmedia>=0.0.0,<2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to move to dev dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the "wagtailmedia" dependency from tox.ini
into pyproject.toml
under the testing label.
@@ -53,6 +53,7 @@ | |||
window.chooserUrls = { | |||
imageChooser: "{% url "wagtailimages_chooser:choose" %}", | |||
documentChooser: "{% url "wagtaildocs_chooser:choose" %}", | |||
mediaChooser: "{% url "wagtailmedia:chooser" %}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this work without the hard dependency, we'll need to move the chooserUrls logic to a template tag and conditionally add mediaChooser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested, I create a new template tag method which checks the wagtailmedia installation and adds the "wagtailmedia:chooser" conditionally into the translation templates. My local tests did work on test environment instance.
@@ -366,6 +367,9 @@ def widget_from_block(block, content_components=None): | |||
elif isinstance(block, ImageChooserBlock): | |||
return {"type": "image_chooser"} | |||
|
|||
elif isinstance(block, AudioChooserBlock) or isinstance(block, VideoChooserBlock): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap in an if apps.is_installed("wagtail.embeds")
and move the block imports inline
if apps.is_installed("wagtail.embeds"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the blocks into an inline import if the wagtailmedia has been installed. The code structure didn't change.
@@ -366,6 +367,9 @@ def widget_from_block(block, content_components=None): | |||
elif isinstance(block, ImageChooserBlock): | |||
return {"type": "image_chooser"} | |||
|
|||
elif isinstance(block, AudioChooserBlock) or isinstance(block, VideoChooserBlock): | |||
return {"type": "media_chooser"} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, need to check whether the direct field (that is the ForeignKey) is to a media class and set the widget type for it as well in widget_from_field()
, a la
wagtail-localize/wagtail_localize/views/edit_translation.py
Lines 321 to 322 in b619a32
elif issubclass(field.related_model, AbstractDocument): | |
return {"type": "document_chooser"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above - same procedure applied here.
…nditionally added mediaChooser to edit translation template
@zerolab Sorry for my late response. I was very busy and occupied with a different project. Nonetheless, I made the requested changes. |
Since the issue #587 still open, i tried to implement this missing feature. We needed this feature to translate the linked media files (Audio only) in our project as soon as possible.
Unfortunately, due to the missing deep knowledge in Wagtail module development process and time constraints, I've only implemented the necessary changes for this feature and skipped the unit tests. Furthermore, I couldn't test the video files, since we only need audio files in our project. I believe that the video files should work fine as well.