search

callumbwhyte / super-value-converters

A collection of powerful property value converters for cleaner code in Umbraco
MIT License
14 stars 12 forks source link

Update MNTP PVC to handle Umbraco special properties #22

Closed NikRimington closed 2 years ago

NikRimington commented 2 years ago

Take the logic from the Umbraco core MNTP PVC around special properties and add it into the SVC version. This allows the expected behaviour to continue to function addressing #21

callumbwhyte commented 2 years ago

Hey @NikRimington,

Thanks for taking the time to contribute to SuperValueConverters! It certainly makes sense that a change like this would be in the package.

Having looked at the code, I'm wondering if a more generic solution could apply? Perhaps the SuperValueConvertersBase class could expose a virtual property to set these values, and handle the logic in there. This would avoid the overridden converters needing to know explicitly what the Core converter returns (e.g. line 50 in your PR) and would instead just defer to calling the Core converter.

Let me know if you think that would solve your issue, and I can get it applied + released today.

Cheers, Callum

NikRimington commented 2 years ago

Hey Callum,

I think there was a reason I didn't try and put it in the base spvc originally as I had a similar thought. I can't remember what it was now, but something didn't work. Could have been my implementation. I'm not advese to it working, but a really nice improvement to the mntp in this package would be to extend the return type of the property to consider these properties as well. I just didn't have time to add that in. But that would also have to be in the specific spvcs I think.

Not sure that really answers your question, sorry.

Nik

callumbwhyte commented 2 years ago

Hey @NikRimington,

I have released v2.2 (for V8) and v3.1 (for V9) just now with this feature included. You will see the SuperValueConverterBase class includes a new overridable IgnoreProperties property.

In a future breaking release I am going to move this property onto the IPickerSettings object.

Hopefully this solution works for you!

Cheers, Callum