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) #227

Conversation

mfendeksilverstripe
Copy link

@mfendeksilverstripe mfendeksilverstripe commented Jan 11, 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

  • some field setups may break without an update

@GuySartorelli
Copy link
Member

Sorry this hasn't been reviewed until now. The concept seems sensible. Are you still interested in having this change merged in?
If so, please retarget to (and rebase on) the 3 branch and add appropriate test coverage for the new API.

@GuySartorelli
Copy link
Member

It seems like there's going to be no further activity on this pull request, so we’ve closed it for now. Please open a new pull-request if you want to re-approach this work, or for anything else you think could be fixed or improved.

@mfendeksilverstripe
Copy link
Author

@GuySartorelli Replaced by #274

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