Skip to content

Add default renderer for form's Forest structure #153

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

Merged
merged 3 commits into from
Jun 2, 2020

Conversation

arthurxavierx
Copy link
Contributor

This can be used for writing form renderers for UI structures that embed Forest.

@arthurxavierx arthurxavierx self-assigned this May 28, 2020
@hdgarrood
Copy link
Contributor

I'm not sure about exporting this from Lumi.Components.Form, since you already need to import .Form.Internal to get hold of a Forest in the first place, don't you? I suppose the same thing applies to the existing defaultRenderForm too.

@arthurxavierx
Copy link
Contributor Author

Yeah, I thought of doing that too but wasn't sure of how I felt about it; that's a good point you've brought up there. You only need to import from .Form.Internal if you need access to the constructors of Forest. The type name is exported from .Form.

I'm ok with moving defaultRenderForest to the internal module, but not sure about moving defaultRenderForm, since, even though being the most logically sound thing to do, would cause a quite unnecessary major version bump and breaking changes.

surround fieldDivider
$ defaultRenderForest { forceTopLabels }
$ Array.mapMaybe pruneTree
$ forest
}
where
fieldDivider = R.hr { className: "lumi field-divider" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defined twice now, not a big deal though

@hdgarrood
Copy link
Contributor

Yeah, agreed. Since defaultRenderForm is already in here I'm tempted to not worry about it. We could also just export the Forest constructors from Form right? I think the fact that we are providing these functions sort of indicates that Forest isn't completely internal to start with.

@@ -166,42 +167,52 @@ defaultRenderForm
}
-> Forest
-> JSX
defaultRenderForm { inlineTable, forceTopLabels } { readonly } forest =
defaultRenderForm renderProps@{ inlineTable, forceTopLabels } { readonly } forest =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not using the renderProps binding here, I think?

@arthurxavierx arthurxavierx merged commit 77566ea into master Jun 2, 2020
@arthurxavierx arthurxavierx deleted the arthur/form/default-render-forest branch June 2, 2020 14:36
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.

3 participants