feat(StrapiFormComponents): add StrapiCheckbox #418

chohner · 2023-11-16T15:11:34Z

Proposal: Mapping the CMS component to React component inside the function (see renderCheckboxFromStrapi()) rather than mapping the props

Co-authored-by: Pram Gurusinga <[email protected]>

frederike-ramin

Puh...I think I get where you're coming from (although, maybe not really - could you explain the reason for changing the structure?)
What I personally don't like about this concept:

We tried to separate components from Strapi as much as possible before, which was a consious decision to declutter the two (Remember when we still had them very close together? I know this doesn't reintroduce the dependency, but definitely is one step in that direction again.
Having a .tsx file within a models folder just feels reeeaaally weird to me and I think might be exactly the sign we're not doing the right thing here.

In the end, it is just another way of translation, so if you still think, it is the better option than a translation function, I'd put the translation (because of the .tsx) closer to the component than the strapi model. - into the component itself or another separate folder (And then the question arises again - what's the advantage here?

chohner · 2023-11-16T15:56:49Z

Puh...I think I get where you're coming from (although, maybe not really - could you explain the reason for changing the structure?)

Main reason is that we are removing one level of complexity (intransparent props destructuring + untypedomitNull()) and avoiding re-running a schema parsing (because all data is already parsed and validated at the edges, like for example inside fetchCollectionEntry()).

We tried to separate components from Strapi as much as possible before, which was a consious decision to declutter the two (Remember when we still had them very close together? I know this doesn't reintroduce the dependency, but definitely is one step in that direction again.

The CMS service (should probably be named strapi service here) is actually the translation layer, and is the only place that should know about how the specifics of the data source map to the app components. Other data sources might write their own translation.

This isn't moving it out of the service but rather closer to the context of each component instead of a generic cmsToReact() function inside of a generic PageContext.tsx file.

Having a .tsx file within a models folder just feels reeeaaally weird to me and I think might be exactly the sign we're not doing the right thing here.

I agree on the naming, but would rather rename models to components or similar. Imo, the translation into React components should actually be the main task of the strapi service; the schemas should technically come from strapi.
Alternatively we could split the schemas and translations but imo they belong so close together that the single file makes sense for me.

frederike-ramin · 2023-11-16T16:17:59Z

app/components/PageContent.tsx

@@ -84,6 +85,8 @@ function cmsToReact(cms: StrapiContent, templateReplacements: Replacements) {
      return <RadioGroup {...getRadioGroupProps(replacedTemplate)} key={key} />;
    case "form-elements.dropdown":
      return <Select {...getDropdownProps(replacedTemplate)} key={key} />;
+    case "form-elements.checkbox":
+      return renderCheckboxFromStrapi(replacedTemplate);


Side note while looking at this - we'll need to also include the key here, right? Otherwise it will result in a log/error?

I was also thinking about this but I don't see the reason for it - the only .map() call happens inside PageContent(), which adds the key for the child <div> element. the elements contained in that don't need the same key again afaik?

frederike-ramin · 2023-11-16T16:23:48Z

Main reason is that we are removing one level of complexity (intransparent props destructuring + untyped omitNull()) and avoiding re-running a schema parsing (because all data is already parsed and validated at the edges, like for example inside fetchCollectionEntry()).

Ah yes, I get it. Especially the double-parsing part.
I still think that the translation feels closer to the components than the cms schema (As it references the Component as is referenced by the PageContent - both in components). but semantically it's closer to the cms.
So in total: I don't have a strong opinion on this. If you do, feel free to change it, but maybe at least add a todo comment to also migrate the other components so that we don't keep two versions of this.