-
Notifications
You must be signed in to change notification settings - Fork 9
Add more input components #54
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
Conversation
seefeldb
commented
Jun 18, 2024
- added ability to set constant props in view()
- added checkbox and textInput that use that
- added checkbox and textInput that use that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, but I think we can simplify the approach taken to specializing input (see comments on third argument to render).
| import { view } from "../hyperscript/render.js"; | ||
| import { eventProps } from "../hyperscript/schema-helpers.js"; | ||
|
|
||
| export const checkbox = view( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would express this as:
export const input = view(
"input",
{
...eventProps(),
type: { type: { anyOf: ["text", "checkbox"] } }
id: { type: "string" },
checked: { type: "boolean" },
value: { type: "string" },
}
);
Input is a weird edge case because the properties are disjoint from the tag type, and the type property acts as a flag that customizes the input properties. We don't currently have a way to express tags that are disjoint from their properties like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see you do this in input.ts. Suggest deleting this file.
| checked: { type: "boolean" }, | ||
| value: { type: "string" }, | ||
| }, | ||
| { type: "checkbox" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note on third argument below.
| value: { type: "string" }, | ||
| placeholder: { type: "string" }, | ||
| }, | ||
| { type: "text" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding a third argument to vnode/render, if we want specialized factories, I would suggest something like:
export const textInput = (props: Props) => input({...props, type: 'text'})| export const view = ( | ||
| tagName: string, | ||
| props: JSONSchemaRecord = {}, | ||
| constantProps: Props = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note on third argument above.