Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses several QA issues including restricting media types for the OneMedia uploader frame, removing dead code, fixing media query issues, load more issues, and filter width issues.
Key Changes:
- Refactored media filtering from taxonomy-based to meta-based queries using
is_onemedia_syncparameter - Added MIME type restrictions to media uploader frames to enforce allowed file types
- Removed duplicate filter hook and dead code (unused import, duplicate AJAX filter, unused CSS rules)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| inc/Modules/MediaLibrary/Admin.php | Removed unused import and duplicate filter hook; refactored attachment query filtering from taxonomy to meta-based approach using is_onemedia_sync parameter |
| assets/src/css/main.scss | Removed unused CSS rules for .onemedia-select-non-sync-media-frame and .onemedia-edit-media-frame selectors |
| assets/src/admin/media-sharing/index.js | Added ALLOWED_MIME_TYPES constant; refactored handleEditMedia to remove unused parameter; added uploader MIME type restrictions for edit media frame |
| assets/src/admin/media-sharing/components/browser-uploader.js | Added is_onemedia_sync parameter to library configurations; implemented MIME type restrictions for both sync and non-sync media upload frames |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
justlevine
left a comment
justlevine
left a comment
There was a problem hiding this comment.
Mainly a couple code quality nits, but I'm not sure I fully understand what we're trying to do with Utils::get_supported_mime_types() https://github.com/rtCamp/OneMedia/pull/16/files#r2645677101
| // Check if is_onemedia_sync is set and true. | ||
| $is_onemedia_sync = isset( $_POST['is_onemedia_sync'] ) && filter_var( wp_unslash( $_POST['is_onemedia_sync'] ), FILTER_VALIDATE_BOOLEAN ); |
There was a problem hiding this comment.
| // Check if is_onemedia_sync is set and true. | |
| $is_onemedia_sync = isset( $_POST['is_onemedia_sync'] ) && filter_var( wp_unslash( $_POST['is_onemedia_sync'] ), FILTER_VALIDATE_BOOLEAN ); | |
| // Handle boolean strings (e.g. 'false'). | |
| $is_onemedia_sync = isset( $_POST['is_onemedia_sync'] ) && filter_var( wp_unslash( $_POST['is_onemedia_sync'] ), FILTER_VALIDATE_BOOLEAN ); |
There was a problem hiding this comment.
I understand the change
justlevine
left a comment
justlevine
left a comment
There was a problem hiding this comment.
Code comment and LGTM (from the diff, I didn't repull to test).
inc/Modules/Settings/Admin.php
Outdated
| public function inject_site_selection_modal(): void { | ||
| $current_screen = get_current_screen(); | ||
| if ( ! $current_screen || 'plugins' !== $current_screen->base ) { | ||
| if ( ! $current_screen || ( 'plugins' !== $current_screen->base && ! str_contains( $current_screen->id, self::MENU_SLUG ) ) ) { |
There was a problem hiding this comment.
Why these changes? As I believe we've discussed previously and multiple times (cc @up1512001), we want the selection modal to show up on any OnePress screen if it hasn't been set.
-> Showing a user an empty screen (or worse a 404)
-> having them guess they need to visit the Plugins screen
-> activate the modal
-> navigate back to the screen they started on
is pure madness.
There was a problem hiding this comment.
Sorry I misread this diff on my phone.
For consistency, and to prevent idiots like me from misreading, lets reuse the should_display_site_selection_modal() helper from
What
onemediauploader frameChecklist