dynamic / silverstripe-flexslider

SilverStripe FlexSlider
BSD 3-Clause "New" or "Revised" License
8 stars 17 forks source link

Specify $owns metadata for slides #165

Closed sminnee closed 5 years ago

sminnee commented 5 years ago

Slides should be "owned" by the parent object. This will mean that when the parent is published, all unpublished slides will get published too, which is the normal expectation for this kind of thing in SilverStripe 4.

Setting this up is a matter of specifying private static $owns, you can see more here: https://docs.silverstripe.org/en/4/developer_guides/model/versioning/#defining-ownership-between-related-versioned-dataobjects

I believe that this is a matter of specifying the following on the FlexSlider data extension, but it would need some testing.

private static $owns = [
    'Slides',
];

I noticed this when using the flexslider elemental block.

muskie9 commented 5 years ago

Thanks for posting this @sminnee, this has been a hot topic over time for some of our modules that have relations. We've had a number of instances in which the $owns was counter to the experience requested by content managers so we've excluded it to this point. That being said I know this tends to be brought up throughout the community for various module.

One thing I did see that I'm not sure we noticed last we checked is the $owned_by which looks at first glance like it would add a bit of flexibility depending on the implementation of the relation and it's particular parent or owner (if that makes sense). I'm wondering if documenting the $owns in this module would be a good middle of the road at this point to keep the option of independent draft/published states, but encouraging the specification of $owns on models that have the FlexSlider extension applied.

We're still figuring out the best way to handle the versioned relationship space so if there's a strong response either way we're happy to accommodate the community. We just want to make sure we beef up our documentation in either case. Let us know your thoughts.

sminnee commented 5 years ago

Yeah, that's an interesting challenge. It's possible that this can be configured in YAML config (I'd have to test, though).

Are the people who prefer the current behaviour using this as an elemental block, or does it only occur with other flexslider use-cases?

muskie9 commented 5 years ago

I believe it's for this module itself and not the elemental module as that is newer. We have had some crossover though from a general element/owns standpoint on projects.

I reached out in the community slack channel and the main feedback I received around this topic is education on content authoring rather than putting the burden on development. That goes both ways though, if we force content authors to archive records they don't want published with the page, then the history viewer support becomes more important if it is something like "seasonal content" or the like.

I think one thing for edge cases, although not the best, could be to remove the config at runtime if we did implement the $owns as most use cases would benefit from the consistency of having it implemented, especially with the elemental module option.

We'll take a look at how this would impact existing implementations and see if it would hinder anything. I don't foresee it causing any major issues and we can probably have something figured out relatively soon from a release standpoint for adding $owns. History viewer enhancements would likely come after through.

sminnee commented 5 years ago

I think our team are looking at history viewer improvements for "object graphs" in general, so what this space!

Certainly we're recommending "$owns on all content" as a standard pattern for SS4 as it fits most users intuitive expectations. Also if some elemental blocks have $owns and others don't it's really confusing :-p

So two paths to solve this would be:

muskie9 commented 5 years ago

I agree, knowing the pattern is moving towards the recursive publishing of everything on a page it's important to stay consistent. I'll regroup with the team and see if there are any objections to implementing the $owns in this module vs silverstripe-elemental-flexslider. I'm guessing implementation on this module will be the path of least resistance.

Do you think it's worth attempting to implement the History Viewer at this point, or are the improvements moving quick enough we would be ok to wait on that enhancement for the module? We've done a tad of graphql and react with our locator module, but there's still a bit of a learning curve we need to tackle.

sminnee commented 5 years ago

Do you think it's worth attempting to implement the History Viewer at this point, or are the improvements moving quick enough we would be ok to wait on that enhancement for the module?

I'll check with the team.

muskie9 commented 5 years ago

@sminnee just curious, what version are you referencing with this request? We have branch 3.0 and 4 (dev-master) as SS 4 compatible.

sminnee commented 5 years ago

These are the versions I had installed. I'm running SS4

dynamic/flexslider 4.0.0-beta3 dynamic/silverstripe-elemental-flexslider 2.0.2 dnadesign/silverstripe-elemental 4.0.3 silverstripe/cms 4.3.3

muskie9 commented 5 years ago

Perfect, I was thinking we could sneak this into v4, but wanted to check if you were using v3.

muskie9 commented 5 years ago

closing in favor of https://github.com/dynamic/silverstripe-elemental-flexslider/issues/22