-
Notifications
You must be signed in to change notification settings - Fork 142
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
Attachment Gallery #508
Attachment Gallery #508
Conversation
Oh gotta sync this up. |
…nt-gallery # Conflicts: # css/shortcode-ui.css # css/shortcode-ui.css.map # dev.php # inc/fields/class-field-attachment.php # js-tests/build/specs.js # js/build/shortcode-ui.js
Updated - think this is ready for a review |
…nto attachment-gallery # Conflicts: # inc/fields/class-field-attachment.php
// .button.remove { | ||
// display: block; | ||
// } | ||
// } |
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.
Should this be commented out?
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.
just what i was looking for
…hment-gallery # Conflicts: # css/shortcode-ui-editor-styles.css.map # css/shortcode-ui.css # css/shortcode-ui.css.map # dev.php # js/build/shortcode-ui.js # js/src/views/edit-attribute-field-attachment.js
Updated. @goldenapples this should be ready to go., |
Seems workable as an initial step here. I'm seeing some weird styling on the Also, I'm not sure if the first issue is actually worked out, is it? When I edit a shortcode after sending it to the editor once, I don't see the previously selected items as selected. It would probably be good to figure that behavior out before merging this, as otherwise that will cause some editorial headaches. |
…hment-gallery # Conflicts: # js/build/shortcode-ui.js # js/src/views/edit-attribute-field-attachment.js
I've pushed some updates for the styles. I'm no longer seeing any issues with the gallery images not loading, although they it does have to fetch the attachment data, so there is a little delay on this, but they should pop in. |
this.$thumbnailDetailsContainer.toggleClass( 'has-attachment', true ); | ||
value.sort( function( a, b ) { | ||
return a < b; | ||
} ); |
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'm not totally sure about sorting the selected attachments by attachment ID, but as we don't have a sortable implementation here yet, this will do as a first introduction to the feature.
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'll have a think about how to tackle this one.
Very nice 💯 This looks and works a lot better for me. I wasn't able to reproduce the issue I was seeing before where selected images are unselected when opening the media frame to select new images. I'm not sure if it was a bug that you fixed, or if I was just confused about the UI before. (Having to command-select to add a selection seems strange, especially if you're on a page of images that don't show any attachments selected already.) The code looks good to me. I'm pretty sure that at some point we'll have to support user-defined ordering of attachments in a gallery, but this is a good chunk of code to merge at one time. I can open a new issue for sortable images, and merge this as is. |
Yes command select images isn't totally obvious, but at least lets us achive the goal. I'll have a look at how to improve this further. Perhaps we can bring in more of the core UI for galleries. |
The `getFromCache()` method was deprecated in Shortcode UI via wp-shortcake/shortcake#508 This should work as a band-aid until fixed upstream via wp-shortcake#75
Support multiple: true on the attachments field.
2 issues i've run into.