Refactoring ideas for consistency

[robbieaverill]

I've noticed a couple of things that are popping up as areas we could refactor:

1. 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.
2. PHP FormField templates: there's three of them now which are basically identical. Perhaps we can $this->renderWith(BaseJSONField::class) or something to centralise those too.
3. React component naming: We have called our PHP FormFields SomeField and the React component Some. We might want to rename the React components to have Field on the end too.
4. CKANResourceLoader component: the other components don't have the CKAN prefix. Should we add it to the other two, or remove it from this one?
5. I've noticed a few places where the CMS field labels (e.g. for filter models) don't match the designs, e.g. "Name" instead of "Filter label" - we need a whip over and to update them so that they do.
6. React component architecture: similarly to the PHP FormFields, these React components all read from initial JSON value prop or state values, and store the state value in a hidden input as serialised JSON. We could centralise some of this logic too, potentially with an HOC, helper functions in a separate file or even a parent component.

Note: when I talk about 3 components, I'm including the one I'm working on now for #25

[ScopeyNZ]

Yeah I'll put something against all of these:

1. 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...
2. Or BaseJsonField 😂 . Yeah considering all components are usually templated in the same way - makes a lot of sense. Ties in a lot with 1)
3. Happy to add Field. Makes sense to me.
4. Yeah this was a hard one when I named it. PHP has namespaces which makes it easy. SilverStripe\CKAN\Thing. JS is harder considering there's no namespacing with JS injector. I named the component I created CKANResourceLoader because I felt like that was the name I'd register with JS injector - it doesn't have to be though. I'd vote for some consistency here - I don't know what's the right way though
5. 👍
6. Yeah that seems like a pretty quick win to me. Updating the value might be a little more ambiguous though I guess. It'll be a bit like redux-form where the HOC has to supply an onChange prop that can be used to mutate the value of the field somehow.

TL;DR: Keen for 1 & 2 - we can do it, but it's also something to consider as a core contribution. 3 & 5 just needs PRs. 4 & 6 read my response.

[robbieaverill]

4. OK, let's rename the other two components to CKAN*

[NightJar]

A little later to the conversation, so picking up from where thing are currently (i.e. not just responding to OP independently)

1. A base "JSONField" or "JSONStoreField" or something - one can the use setComponent or define services for using different react components.
2. See 1.
3. Sure.
4. I concur.
5. I named things how I thought made sense in terms of an abstraction (i.e. moving the core CKAN interfacing stuff to another module to decouple it from the CKAN Registry front-end). To this end they don't match designs - I thought about using translation in this module to control that... but it's kind of all or nothing as @ScopeyNZ pointed out to me (translates everywhere, not just in a particular context). Yes, I agree renaming to match designs is needed - I guess I'm just trying to say that we should be careful to confine it to registry specific code, rather than introduce coupling between the interfacing code and the registry implementation. (i.e hence a generic "Field" as opposed to "Column").
6. Sounds good - I've been busy in PHP land so don't have much context. Happy with whatever decision, but I'll add it seems silly to not do it as described :)

[robbieaverill]

I'm just trying to say that we should be careful to confine it to registry specific code, rather than introduce coupling between the interfacing code and the registry implementation. (i.e hence a generic "Field" as opposed to "Column").

To be, they both sound equally generic - are you matching them against some standard CKAN naming scheme or something?

IMO our priority needs to be on matching the designs we've been given unless otherwise agreed (and designs updated) otherwise we're all going to get confused at some stage

[NightJar]

I was thinking about cartographic resources in particular, in that a map does not have the concept of a column.
I don't disagree that the UI needs to match the designs :)

[NightJar]

1. consolidate JSON saving form fields with a base class #65
2. Also consolidate JSON saving form fields with a base class #65
3. React component naming consistency #66
4. Also React component naming consistency #66
5. Update CMS field labels to match designs for CKAN-Registry representation #67
6. Abstract JSON saving React form components to have a common back-end #68

As these items are all separated out now, I'll close this issue.