Feature attachment details modal #202
Conversation
|
The PHP logic should be in a separate PHP class, not in foogallery.php. Example : includes/admin/class-attachment-modal.php. The CSS can be moved into css/admin-foogallery.css I want this feature to be able to be turned on/off in settings, so that if there are issues with the modal logic, the user can revert back to how it works at the moment with an admin setting. This will mean a check in the JS to allow the old way of working and the new to also work based on a setting. |
|
@bradvin The Feedback for this PR is attended and pushed to the git though commit "PR 202 Changes", please have a look! |
| $foogallery_button_url = get_post_meta( $img_id, '_foogallery_button_url', true ); | ||
| $foogallery_product = get_post_meta( $img_id, '_foogallery_product', true ); | ||
| $panning = ''; | ||
| $progress = get_post_meta( $gallery_id, FOOGALLERY_META_WATERMARK_PROGRESS, true ); | ||
| $category_taxonomy = 'foogallery_attachment_category'; | ||
| $tag_taxonomy = 'foogallery_attachment_tag'; |
There was a problem hiding this comment.
rather use constant FOOGALLERY_ATTACHMENT_TAXONOMY_TAG
|
There is a lot of logic in function open_foogallery_image_edit_modal_ajax. Both for setting variables and for displaying the tabs. Break this up into separate private functions for each tab. 1 function to set the variables and another for displaying the tab. This might mean you would need to set all the variables in a class level private array. And then in the display methods, you pull data from the array. (I hope this makes sense?) This way, the code is neater and easier to maintain. Also, you can also conditionally call the functions for tabs that are in pro features (taxonomies, exif, woocommerce etc). I am thinking that all the tabs for free features are included in this file, and then a custom action hook is called. Then for example, specific logic for EXIF can be moved to the pro exif class file. Similar for taxonomies etc. (for now, keep all the pro functions in this file separated out, and then move them to their final destination files in a later commit.) |
|
The modal takes a while to load (due to all the data it is fetching etc) witch is understandable. Rather show a blank modal immediately after clicking on the "i" for a thumb. And show a loading spinner while it is loading everything behind the scenes. |
foogallery.php
Outdated
| foreach ( $img_tags as $tag ) { | ||
| $selected_tags[] = $tag->term_id; | ||
| } | ||
| $categories = $terms = get_terms( array( |
| 'foogallery_crop_pos_val' => 'center,center', | ||
| 'foogallery_override_thumbnail' => '', | ||
| ); | ||
|
|
||
| public function __construct() { | ||
| add_action( 'wp_ajax_open_foogallery_image_edit_modal', array( $this, 'open_foogallery_image_edit_modal_ajax' ) ); | ||
| add_action( 'admin_footer', array( $this, 'foogallery_image_editor_modal' ) ); | ||
| add_filter( 'foogallery_attachment_custom_fields', array( $this, 'foogallery_add_override_thumbnail_field' ) ); | ||
| add_action( 'wp_ajax_foogallery_save_modal_metadata', array( $this, 'foogallery_save_modal_metadata' ) ); |
There was a problem hiding this comment.
rename function foogallery_save_modal_metadata to ajax_save_modal
| 'foogallery_crop_pos_val' => 'center,center', | ||
| 'foogallery_override_thumbnail' => '', | ||
| ); | ||
|
|
||
| public function __construct() { | ||
| add_action( 'wp_ajax_open_foogallery_image_edit_modal', array( $this, 'open_foogallery_image_edit_modal_ajax' ) ); |
There was a problem hiding this comment.
rename function open_foogallery_image_edit_modal_ajax to ajax_open_modal
| @@ -410,7 +138,7 @@ public function foogallery_add_override_thumbnail_field( $fields ) { | |||
| } | |||
|
|
|||
| public function foogallery_save_modal_metadata() { | |||
There was a problem hiding this comment.
this function is not secure. It needs a call to wp_verify_nonce
| wp_die(); | ||
| } | ||
|
|
||
| private function set_image_modal_vars( $args = array() ) { |
There was a problem hiding this comment.
this function needs to be split into multiple functions, based on the separate tabs, like it was done for the tab content.
Use a filter to hook into and set the variables for the different tabs. Something like:
$this->img_modal = apply_filters( 'foogallery_attachment_modal_data', $this->img_modal, $args );
| <?php do_action( 'foogallery_img_modal_tabs_view' ); ?> | ||
| </div> | ||
| <div class="tab-panels"> | ||
| <?php do_action( 'foogallery_img_modal_tab_content', $_POST ); ?> |
There was a problem hiding this comment.
pass $this->img_modal into the action rather than $_POST
| <?php } | ||
|
|
||
| public function foogallery_img_modal_tab_content_main( $args = array() ) { | ||
| $img_modal = $this->set_image_modal_vars( $args ); |
There was a problem hiding this comment.
if $this->img_modal is passed into this function, then you do not need to do this.
| <section id="foogallery-panel-main" class="tab-panel active" data-nonce="<?php echo $this->img_modal['nonce'];?>"> | ||
| <div class="settings"> | ||
| <span class="setting" data-setting="title"> | ||
| <label for="attachment-details-two-column-title" class="name">Title</label> |
There was a problem hiding this comment.
use i8ln functions for all text. For example in this case:
<?php _e('Title', 'foogallery'); ?>
| </select> | ||
| </span> | ||
| <span class="setting" data-setting="custom_class"> | ||
| <label for="attachments-foogallery-custom-class" class="name">Custom Class</label> |
There was a problem hiding this comment.
Please replace all text with i8ln functions. refer to the coding standards
|
Some changes that are needed, after review. I will add a comment for each one Save refactor: saving the modal data is done at different times at the moment (when input's change etc). Include a 'Save Attachment Details' button at bottom right corner of the modal. When the button is clicked, a spinner must be shown next to the button; all the data from all tabs can be scraped into an array; all data sent to the server. The save function can call a do_action and then similar to how tab display is split into functions, the saving can also be split into different functions per tab. |
|
Change the left image section:
|
|
The selected tab should not have a line underneath: |
|
Generate Watermark Image - at the moment, this generates ALL watermarks for all the attachments in the gallery. This should rather only generate the watermark image for the current image we are viewing the modal for. A new ajax function should be added to generate the watermark. Within this function, you can do something like this: Please note I had to change the FooGallery_Pro_Protection class to make the generate_watermark public, so please pull code (ade284c) After the watermark is successfully generated, then show the image |
|
EXIF data is being saved to so that it can be picked up by the EXIF feature. Look in class-foogallery-pro-exif.php |
|
Alternate Thumbnail changes:
|
|
Add the ability to view attachment details for the next/prev attachment within the gallery. This is done on the attachment edit modal built into WP:
The next / previous attachment ID will need to be calculated and passed to the modal so that these buttons can work |
…/Code-BeautifulWeb/foogallery into feature-attachment-details-modal
@bradvin Please have a look on recent Commits for Image Attachment Modal feature!