Skip to content
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

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

Open
wants to merge 1 commit into
base: 3
Choose a base branch
from

Conversation

mfendeksilverstripe
Copy link

@mfendeksilverstripe mfendeksilverstripe commented Dec 21, 2023

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

@GuySartorelli
Copy link
Member

GuySartorelli commented Dec 21, 2023

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.

@GuySartorelli GuySartorelli marked this pull request as draft December 21, 2023 20:48
@mfendeksilverstripe
Copy link
Author

@GuySartorelli Replaces #227

@mfendeksilverstripe
Copy link
Author

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

@GuySartorelli
Copy link
Member

@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 #274 (comment) 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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants