mfendeksilverstripe commented 11 months ago

ENH: Configuration enhancement (search, value sort fields, allow raw data storage)

added the ability to specify search, value and sort fields which were previously hard coded
added the ability to store raw data to cover the cases where data serialisation is done on the model level as opposed to the controller level (form)

This change is mostly code reuse from one of my projects so I would be keen on getting early feedback before sinking too much effort into this (docs, unit tests, backwards compatibility...). I'm mostly interested to first validate if we need to make this change. Here are some topics I would like to cover:

responsibility of this form field (should serialisation be covered or not)
"correct setup" detection (some of the code is making assumptions instead of forcing the developer to properly configure the field)

Backwards compatibility considerations

changes should be backwards compatible

Related issues

https://github.com/silverstripe/silverstripe-tagfield/issues/275

GuySartorelli commented 11 months ago

Please create an issue, rather than a pull request, for discussions about this sort of thing (as per our contribution guide)

Please include as much information as possible about the intended use case in the issue that you create - as it is, I'm not sure what you mean by "data serialisation is done on the model level as opposed to the controller level (form)" for example.

I'm also marking this as draft since it's not intended to be merged as-is.

mfendeksilverstripe commented 11 months ago

@GuySartorelli Replaces https://github.com/silverstripe/silverstripe-tagfield/pull/227

mfendeksilverstripe commented 11 months ago

@GuySartorelli I've implemented your feedback from the previous version of this PR, please review.

GuySartorelli commented 11 months ago

@mfendeksilverstripe Based on the description it sounds like you're more after a discussion than a peer review on this PR, is that correct? If so, please see https://github.com/silverstripe/silverstripe-tagfield/pull/274#issuecomment-1866911081 for how to better get that discussion going.

I don't have time to review this PR this side of Christmas, so if it is just a review of this PR as it is, please update the description to indicate that and for best results remind me when business starts again next year.

mfendeksilverstripe commented 10 months ago

Hey @GuySartorelli I think I've addressed all your feedback. Can you please have another look?

silverstripe / silverstripe-tagfield