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

Gallery field #174

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@danielbachhuber
Collaborator

danielbachhuber commented Jul 27, 2014

I think we might've discussed this before, but I don't see an existing thread for it.

How would you feel about a gallery field that used the Media Library to pick a selection of images? Using a repeating / sortable media field is a bit janky UX.

@netaustin

This comment has been minimized.

Show comment
Hide comment
@netaustin

netaustin Jul 26, 2014

Member

I would welcome such a thing. It's on my long horizon, just over the 100% test coverage hill.

Member

netaustin commented Jul 26, 2014

I would welcome such a thing. It's on my long horizon, just over the 100% test coverage hill.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 27, 2014

Collaborator

@netaustin Before I proceed too much further here, I'd love to get your input. UX looks a bit like this (ignore the Media Explorer buttons that are being added):

image

image

image

A collection attribute makes most sense, I think, but it's exposed some shakiness in the underlying Media field and I probably need to refactor the whole thing.

I need gallery management for a couple of clients within two or three weeks, so I'm hoping we can turn this around pretty quick.

Collaborator

danielbachhuber commented Jul 27, 2014

@netaustin Before I proceed too much further here, I'd love to get your input. UX looks a bit like this (ignore the Media Explorer buttons that are being added):

image

image

image

A collection attribute makes most sense, I think, but it's exposed some shakiness in the underlying Media field and I probably need to refactor the whole thing.

I need gallery management for a couple of clients within two or three weeks, so I'm hoping we can turn this around pretty quick.

@bcampeau

This comment has been minimized.

Show comment
Hide comment
@bcampeau

bcampeau Jul 27, 2014

Member

This would actually solve a use case for me on a couple sites we've worked on. We've just had them use the standard gallery function via the media modal and generate a shortcode because as you said the repeating media field UX isn't efficient. However this locks down the functionality so an editor could only use the field for this purpose which is great. I like this.

Member

bcampeau commented Jul 27, 2014

This would actually solve a use case for me on a couple sites we've worked on. We've just had them use the standard gallery function via the media modal and generate a shortcode because as you said the repeating media field UX isn't efficient. However this locks down the functionality so an editor could only use the field for this purpose which is great. I like this.

@netaustin

This comment has been minimized.

Show comment
Hide comment
@netaustin

netaustin Jul 27, 2014

Member

Proceed apace, yeah. The underlying media field was built for the earlier version of the media modal, which was a serious pain to work with, and then mildly refactored when the media browser changed. Probably needs a more serious cleaning.

Member

netaustin commented Jul 27, 2014

Proceed apace, yeah. The underlying media field was built for the earlier version of the media modal, which was a serious pain to work with, and then mildly refactored when the media browser changed. Probably needs a more serious cleaning.

@mboynes

This comment has been minimized.

Show comment
Hide comment
@mboynes

mboynes Aug 31, 2014

Member

I really dig this idea. Where are you at with it @danielbachhuber? Need any assistance?

Member

mboynes commented Aug 31, 2014

I really dig this idea. Where are you at with it @danielbachhuber? Need any assistance?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 31, 2014

Collaborator

Where are you at with it @danielbachhuber? Need any assistance?

60% done. Just need time to finish it up. What sort of test coverage are you looking for?

Collaborator

danielbachhuber commented Aug 31, 2014

Where are you at with it @danielbachhuber? Need any assistance?

60% done. Just need time to finish it up. What sort of test coverage are you looking for?

@mboynes

This comment has been minimized.

Show comment
Hide comment
@mboynes

mboynes Aug 31, 2014

Member

We don't have testing for JS (#10), so just the php functionality. Ensure that the field renders properly when setting 'collection' => true, ensure that the values save, and when loading the form, the values load properly. If there's any sort of circumstance where the values could be invalid, the tests should cover that too. There's some good precedent in this file.

Member

mboynes commented Aug 31, 2014

We don't have testing for JS (#10), so just the php functionality. Ensure that the field renders properly when setting 'collection' => true, ensure that the values save, and when loading the form, the values load properly. If there's any sort of circumstance where the values could be invalid, the tests should cover that too. There's some good precedent in this file.

mattheu and others added some commits Jan 6, 2015

Merge branch 'master' of github.com:mattheu/wordpress-fieldmanager in…
…to media-collection-174

Conflicts:
	js/media/fieldmanager-media.js
	php/class-fieldmanager-media.php
Merge branch 'master' of github.com:mattheu/wordpress-fieldmanager in…
…to media-collection-174-2

Conflicts:
	js/media/fieldmanager-media.js
	php/class-fieldmanager-media.php
Merge branch 'media-collection-174-2' into media-collection-174-mods
Conflicts:
	js/media/fieldmanager-media.js
	php/class-fieldmanager-media.php
Merge branch 'master' of github.com:alleyinteractive/wordpress-fieldm…
…anager into media-collection-174-mods

Conflicts:
	css/fieldmanager.css
Merge branch 'media-collection-174' of github.com:alleyinteractive/wo…
…rdpress-fieldmanager into media-collection-174-mods
'type': 'image',
'post__in': selectedImages,
'orderby': 'post__in',
'perPage': -1

This comment has been minimized.

@danielbachhuber

danielbachhuber Jan 12, 2015

Collaborator

Not sure how VIP is going to feel about this. Might need to add some transparent pagination.

@danielbachhuber

danielbachhuber Jan 12, 2015

Collaborator

Not sure how VIP is going to feel about this. Might need to add some transparent pagination.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 12, 2015

Collaborator

@mattheu Couple of things still:

  • If I'm using an image field with limit => 0, as Fusion is, I'd expect I could use collection => true to turn all of those images into a gallery. The behavior as it exists in this PR is to implement repeatable collections, which introduces a new data structure.
  • When I edit the collection, I'd expect it to open the edit gallery experience, not the UI to select images again.

@mboynes @netaustin still interested in this PR, or should we internalize first? what's the backlog like getting something onto VIP?

Collaborator

danielbachhuber commented Jan 12, 2015

@mattheu Couple of things still:

  • If I'm using an image field with limit => 0, as Fusion is, I'd expect I could use collection => true to turn all of those images into a gallery. The behavior as it exists in this PR is to implement repeatable collections, which introduces a new data structure.
  • When I edit the collection, I'd expect it to open the edit gallery experience, not the UI to select images again.

@mboynes @netaustin still interested in this PR, or should we internalize first? what's the backlog like getting something onto VIP?

@mattheu

This comment has been minimized.

Show comment
Hide comment
@mattheu

mattheu Jan 13, 2015

Contributor

Currently you can switch from limit = 0 and collection=false to limit=false and collection=true as the data structure is the same here.

Also If you click a collection to edit - it does already open the edit gallery experience...

Contributor

mattheu commented Jan 13, 2015

Currently you can switch from limit = 0 and collection=false to limit=false and collection=true as the data structure is the same here.

Also If you click a collection to edit - it does already open the edit gallery experience...

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 13, 2015

Collaborator

Hmm. Maybe I didn't pull latest. I'll test again today

Collaborator

danielbachhuber commented Jan 13, 2015

Hmm. Maybe I didn't pull latest. I'll test again today

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 16, 2015

Collaborator

My bad on testing — the issues are non-issues.

Because we need this now, we've moved it to a separate repo: https://github.com/fusioneng/fieldmanager-gallery

Happy to pick things up again when there's some direction from the maintainers on where we'd like this to go.

Collaborator

danielbachhuber commented Jan 16, 2015

My bad on testing — the issues are non-issues.

Because we need this now, we've moved it to a separate repo: https://github.com/fusioneng/fieldmanager-gallery

Happy to pick things up again when there's some direction from the maintainers on where we'd like this to go.

@mboynes

This comment has been minimized.

Show comment
Hide comment
@mboynes

mboynes Jun 26, 2015

Member

@danielbachhuber I'd love to finally merge https://github.com/fusioneng/fieldmanager-gallery into FM. Your thoughts? I'm inclined to keep it a separate field rather than add onto Fieldmanager_Media as you did initially. One potential issue is naming -- we don't want to break any sites where your plugin is installed.

Member

mboynes commented Jun 26, 2015

@danielbachhuber I'd love to finally merge https://github.com/fusioneng/fieldmanager-gallery into FM. Your thoughts? I'm inclined to keep it a separate field rather than add onto Fieldmanager_Media as you did initially. One potential issue is naming -- we don't want to break any sites where your plugin is installed.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jun 26, 2015

Collaborator

Your thoughts?

I'm open to it.

I'm inclined to keep it a separate field rather than add onto Fieldmanager_Media as you did initially.

Seems reasonable.

One potential issue is naming -- we don't want to break any sites where your plugin is installed.

I suspect this is a half dozen or less. We haven't really given it much promotion, and we only have one site. My bad not changing the name in the first place.

Collaborator

danielbachhuber commented Jun 26, 2015

Your thoughts?

I'm open to it.

I'm inclined to keep it a separate field rather than add onto Fieldmanager_Media as you did initially.

Seems reasonable.

One potential issue is naming -- we don't want to break any sites where your plugin is installed.

I suspect this is a half dozen or less. We haven't really given it much promotion, and we only have one site. My bad not changing the name in the first place.

@mboynes mboynes modified the milestone: later Feb 20, 2016

@mboynes mboynes modified the milestones: later, 1.1.0 Feb 22, 2016

@enrico-sorcinelli

This comment has been minimized.

Show comment
Hide comment
@enrico-sorcinelli

enrico-sorcinelli Jul 9, 2016

Contributor

I just sent a new PR with an updated Fieldmanager gallery version (with unit tests) in order to finally close this thread ;-)

Contributor

enrico-sorcinelli commented Jul 9, 2016

I just sent a new PR with an updated Fieldmanager gallery version (with unit tests) in order to finally close this thread ;-)

@mboynes

This comment has been minimized.

Show comment
Hide comment
@mboynes

mboynes Oct 4, 2016

Member

Closing in favor of #506

Member

mboynes commented Oct 4, 2016

Closing in favor of #506

@mboynes mboynes closed this Oct 4, 2016

@mboynes mboynes removed this from the 1.2.0 milestone Oct 10, 2017

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