This repository has been archived by the owner. It is now read-only.

Gallery Widget #120

Merged
merged 43 commits into from Sep 19, 2017

Conversation

6 participants
@obenland
Collaborator

obenland commented May 2, 2017

obenland added some commits May 2, 2017

[WIP] Gallery Widget
First pass.

Fixes #62.
Add preview for galleries
Very basic, needs much more polish. And styles.
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 3, 2017

Member

Should we not continue with array but convert between string and array in the prop-mapping functions?

Should we not continue with array but convert between string and array in the prop-mapping functions?

This comment has been minimized.

Show comment
Hide comment
@obenland

obenland May 4, 2017

Collaborator

That's definitely a possibility. I just ran into an array to string conversion error with an empty array as a default and haven't had time to debug that

Collaborator

obenland replied May 4, 2017

That's definitely a possibility. I just ran into an array to string conversion error with an empty array as a default and haven't had time to debug that

@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc May 3, 2017

Collaborator

Looks like @westonruter already commented but I saw this too:

<br />↵<b>Notice</b>: Array to string conversion in <b>/srv/www/wordpress-develop/public_html/src/wp-content/plugins/wp-core-media-widgets/wp-includes/widgets/class-wp-widget-media.php</b> on line <b>330</b><br />↵Array

Which was resulting in some JS errors in the preview render. I added some logic to ensure we are working against an array in the preview, but agree we should maybe consider returning just the image urls needed in attachments? Not entirely sure the best thing to do.

🤞 that 905edbc also clears up the travis issues.

Collaborator

timmyc commented May 3, 2017

Looks like @westonruter already commented but I saw this too:

<br />↵<b>Notice</b>: Array to string conversion in <b>/srv/www/wordpress-develop/public_html/src/wp-content/plugins/wp-core-media-widgets/wp-includes/widgets/class-wp-widget-media.php</b> on line <b>330</b><br />↵Array

Which was resulting in some JS errors in the preview render. I added some logic to ensure we are working against an array in the preview, but agree we should maybe consider returning just the image urls needed in attachments? Not entirely sure the best thing to do.

🤞 that 905edbc also clears up the travis issues.

@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc May 3, 2017

Collaborator

I disabled the code sniffer in a few lines as it was returning the following error about rand:

wp-includes/widgets/class-wp-widget-media-gallery.php:112:38: error - Detected forbidden query_var "orderby" of "rand". Use vip_get_random_posts() instead. (WordPress.VIP.OrderByRand.orderby_orderby)

Figured that didn't really apply here.

Collaborator

timmyc commented May 3, 2017

I disabled the code sniffer in a few lines as it was returning the following error about rand:

wp-includes/widgets/class-wp-widget-media-gallery.php:112:38: error - Detected forbidden query_var "orderby" of "rand". Use vip_get_random_posts() instead. (WordPress.VIP.OrderByRand.orderby_orderby)

Figured that didn't really apply here.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 3, 2017

Member

I disabled the code sniffer in a few lines as it was returning the following error about rand

@timmyc feel free to give the WordPress.VIP.OrderByRand.orderby_orderby error a severity of 0 in the underlying phpcs.xml.dist instead.

Member

westonruter commented May 3, 2017

I disabled the code sniffer in a few lines as it was returning the following error about rand

@timmyc feel free to give the WordPress.VIP.OrderByRand.orderby_orderby error a severity of 0 in the underlying phpcs.xml.dist instead.

@westonruter westonruter referenced this pull request May 22, 2017

Merged

Add Gallery Block #740

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter May 22, 2017

Member

Cross-reference Gallery block: WordPress/gutenberg#740

Member

westonruter commented May 22, 2017

Cross-reference Gallery block: WordPress/gutenberg#740

obenland and others added some commits May 2, 2017

[WIP] Gallery Widget
First pass.

Fixes #62.
Add preview for galleries
Very basic, needs much more polish. And styles.
m1tk00-devv m1tk00-devv
Fix preview for gallery
/**
 * TODO fix edit button
 */
@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc Aug 9, 2017

Collaborator

@m1tk00 - excellent, I am not getting errors in the preview anymore!! Outside of the linting issues we are having on the branch now - the following flow appears to be broken for me:

  • Create a new Gallery Widget, assign photos, finish the modal wizard
  • Click "Edit Gallery" in the widget. Note the current photos are not selected, and the modal state shown is the same as creating a new gallery.

In this scenario, I expected to be shown the same dialog state as seen when clicking the 'pencil' to edit a gallery within the editor:

add_new_post_ _wordpress_develop_ _wordpress

Collaborator

timmyc commented Aug 9, 2017

@m1tk00 - excellent, I am not getting errors in the preview anymore!! Outside of the linting issues we are having on the branch now - the following flow appears to be broken for me:

  • Create a new Gallery Widget, assign photos, finish the modal wizard
  • Click "Edit Gallery" in the widget. Note the current photos are not selected, and the modal state shown is the same as creating a new gallery.

In this scenario, I expected to be shown the same dialog state as seen when clicking the 'pencil' to edit a gallery within the editor:

add_new_post_ _wordpress_develop_ _wordpress

@m1tk00

This comment has been minimized.

Show comment
Hide comment
@m1tk00

m1tk00 Aug 9, 2017

Collaborator

@timmyc yeah thats where I got stuck. I tried passing the data via the selected option but can't get it working

Collaborator

m1tk00 commented Aug 9, 2017

@timmyc yeah thats where I got stuck. I tried passing the data via the selected option but can't get it working

@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc Aug 14, 2017

Collaborator

@m1tk00 so looking at how the Gutenberg Gallery block performs the operation: https://github.com/WordPress/gutenberg/blob/master/blocks/media-upload-button/index.js#L124-L141 - they hook into the onOpen event then add in the selected items at that point. Do you want to give that a try or do you want me to work on that here?

Collaborator

timmyc commented Aug 14, 2017

@m1tk00 so looking at how the Gutenberg Gallery block performs the operation: https://github.com/WordPress/gutenberg/blob/master/blocks/media-upload-button/index.js#L124-L141 - they hook into the onOpen event then add in the selected items at that point. Do you want to give that a try or do you want me to work on that here?

@m1tk00

This comment has been minimized.

Show comment
Hide comment
@m1tk00

m1tk00 Aug 14, 2017

Collaborator
Collaborator

m1tk00 commented Aug 14, 2017

m1tk00-devv m1tk00-devv
@m1tk00

This comment has been minimized.

Show comment
Hide comment
@m1tk00

m1tk00 Aug 15, 2017

Collaborator

@timmyc Ive managed to fix the edit button. It was calling a wrong media frame. Just pushed to the repo.

Collaborator

m1tk00 commented Aug 15, 2017

@timmyc Ive managed to fix the edit button. It was calling a wrong media frame. Just pushed to the repo.

@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc Aug 15, 2017

Collaborator

@m1tk00 I'm still having issues with the edit flow - my selections are not being shown when clicking the Edit button.

Collaborator

timmyc commented Aug 15, 2017

@m1tk00 I'm still having issues with the edit flow - my selections are not being shown when clicking the Edit button.

@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc Aug 16, 2017

Collaborator

So the past 3 commits are me trying to get CI to pass :) - each time, there is an array test that seems to go 💥 like the latest:

Parse error: syntax error, unexpected T_STRING in /tmp/wpcs/WordPress/Sniffs/Arrays/CommaAfterArrayItemSniff.php on line 10

Any ideas @westonruter? I tried to re-create the issue locally and could not, then again my phpcs knowledge is limited.

Collaborator

timmyc commented Aug 16, 2017

So the past 3 commits are me trying to get CI to pass :) - each time, there is an array test that seems to go 💥 like the latest:

Parse error: syntax error, unexpected T_STRING in /tmp/wpcs/WordPress/Sniffs/Arrays/CommaAfterArrayItemSniff.php on line 10

Any ideas @westonruter? I tried to re-create the issue locally and could not, then again my phpcs knowledge is limited.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 16, 2017

Member

I'm working on some improvements to the rendering and syncing of the gallery settings.

Member

westonruter commented Sep 16, 2017

I'm working on some improvements to the rendering and syncing of the gallery settings.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 16, 2017

Member

@joemcgill there's one key thing thing left before I think we can call this ready to merge: customizations.

When you have selected images for a gallery, you then see something like this:

image

If you try supplying a caption then:

image

And then it update, there is no changes made because there is no attachments in the model there is nowhere for the customizations to go. Normally in the post editor when you edit a caption it then gets written back to the underlying attachment. However, in the case of media widgets this is blocked entirely with this logic:

// Disable syncing of attachment changes back to server (except for deletions). See <https://core.trac.wordpress.org/ticket/40403>.
defaultSync = wp.media.model.Attachment.prototype.sync;
wp.media.model.Attachment.prototype.sync = function( method ) {
	if ( 'delete' === method ) {
		return defaultSync.apply( this, arguments );
	} else {
		return $.Deferred().rejectWith( this ).promise();
	}
};
mediaFrame.on( 'close', function onClose() {
	wp.media.model.Attachment.prototype.sync = defaultSync;
});

It seems that for the Gallery widget we must remove this logic or else a user will not be able to modify the image attributes.

Member

westonruter commented Sep 16, 2017

@joemcgill there's one key thing thing left before I think we can call this ready to merge: customizations.

When you have selected images for a gallery, you then see something like this:

image

If you try supplying a caption then:

image

And then it update, there is no changes made because there is no attachments in the model there is nowhere for the customizations to go. Normally in the post editor when you edit a caption it then gets written back to the underlying attachment. However, in the case of media widgets this is blocked entirely with this logic:

// Disable syncing of attachment changes back to server (except for deletions). See <https://core.trac.wordpress.org/ticket/40403>.
defaultSync = wp.media.model.Attachment.prototype.sync;
wp.media.model.Attachment.prototype.sync = function( method ) {
	if ( 'delete' === method ) {
		return defaultSync.apply( this, arguments );
	} else {
		return $.Deferred().rejectWith( this ).promise();
	}
};
mediaFrame.on( 'close', function onClose() {
	wp.media.model.Attachment.prototype.sync = defaultSync;
});

It seems that for the Gallery widget we must remove this logic or else a user will not be able to modify the image attributes.

joemcgill added some commits Sep 16, 2017

Use all gallery shortcode properties when rendering shortcode.
This passes all of the gallery properties as shortcode atts when
rendereing the gallery on the front-end.
@joemcgill

This comment has been minimized.

Show comment
Hide comment
@joemcgill

joemcgill Sep 16, 2017

Collaborator

@westonruter I was looking at that syncing logic yesterday and agree with you that in this context, captions should be able to be synced back to the underlying attachment. Your updates to the syncing of props between the widget control and the media frame seem to fix the issues I had noticed but the gallery attributes weren't being used to render the gallery shortcode on the front end, so I pushed another quick commit to resolve that issue.

Will do some more testing, but other than the caption issue, things are looking good from my end. Just a note that captions are a bit tricky because some themes disregard captions when they render galleries.

Collaborator

joemcgill commented Sep 16, 2017

@westonruter I was looking at that syncing logic yesterday and agree with you that in this context, captions should be able to be synced back to the underlying attachment. Your updates to the syncing of props between the widget control and the media frame seem to fix the issues I had noticed but the gallery attributes weren't being used to render the gallery shortcode on the front end, so I pushed another quick commit to resolve that issue.

Will do some more testing, but other than the caption issue, things are looking good from my end. Just a note that captions are a bit tricky because some themes disregard captions when they render galleries.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 16, 2017

Member

@joemcgill excellent, thanks. Once you merge this PR then we'll release a new version of the feature plugin to get out to testers for next week prior to core merge.

Member

westonruter commented Sep 16, 2017

@joemcgill excellent, thanks. Once you merge this PR then we'll release a new version of the feature plugin to get out to testers for next week prior to core merge.

@joemcgill

This comment has been minimized.

Show comment
Hide comment
@joemcgill

joemcgill Sep 19, 2017

Collaborator

@westonruter Got a chance to this again after e5ee972, and captions updates are now saving back to the model correctly. Going to go ahead and merge so we can get this into more hands. Nice work 🎉

Collaborator

joemcgill commented Sep 19, 2017

@westonruter Got a chance to this again after e5ee972, and captions updates are now saving back to the model correctly. Going to go ahead and merge so we can get this into more hands. Nice work 🎉

@joemcgill joemcgill merged commit cec2c4f into master Sep 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 19, 2017

Member

Great work everyone!

Member

westonruter commented Sep 19, 2017

Great work everyone!

@westonruter westonruter deleted the add/gallery-widget branch Sep 19, 2017

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 19, 2017

Member

The plugin auto-deployed to WordPress.org so it is now available for all to test: https://wordpress.org/plugins/wp-core-media-widgets/

Member

westonruter commented Sep 19, 2017

The plugin auto-deployed to WordPress.org so it is now available for all to test: https://wordpress.org/plugins/wp-core-media-widgets/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.