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

consolidate JSON saving form fields with a base class #65

Open
NightJar opened this issue Jan 6, 2019 · 4 comments
Open

consolidate JSON saving form fields with a base class #65

NightJar opened this issue Jan 6, 2019 · 4 comments

Comments

@NightJar
Copy link
Contributor

NightJar commented Jan 6, 2019

As per point 1 in #57

@robbieaverill points out

PHP FormField implementations: all three FormFields are essentially composite React component fields which read and write their value via serialised JSON. We could probably centralise some of the logic for this between the three classes.

@ScopeyNZ follows up

Good idea. I almost feel like there could be something in core for this JsonComponentField or something. You could just extend it and define what component is used...

@NightJar continues

A base "JSONField" or "JSONStoreField" or something - one can the use setComponent or define services for using different react components.

In summary
Create a form field type that will save JSON blobs to the database. Then continue to replace ResourceLocatorField, ResultConditionsField, and PresentedOptionsField (in an upcoming PR) with this base field, setting the react component property to use the relevant view.

@robbieaverill
Copy link
Contributor

I don't think it makes sense to do this until #25 is completed, since ResultConditionsField just stores its JSON value as a string which is entirely handled on the frontend - there's no logic there. We'll otherwise be refactoring for one use case (ResourceLocatorField) at this stage.

@robbieaverill
Copy link
Contributor

Similarly to the other issue, do we still want to do this? I'm happy to close it

@ScopeyNZ
Copy link
Contributor

I think it's not that important on this repo. It's a good idea for silverstripe-admin though.

@NightJar
Copy link
Contributor Author

I think it's not necessary now, but I don't think it's worth closing either (unless as @ScopeyNZ says, it's moved to silverstripe/admin instead). It's certainly something that should be tracked.

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

No branches or pull requests

4 participants