Skip to content

Conversation

@seefeldb
Copy link
Contributor

  • added ability to set constant props in view()
  • added checkbox and textInput that use that

- added checkbox and textInput that use that
@seefeldb seefeldb requested a review from gordonbrander June 18, 2024 17:32
Copy link
Contributor

@gordonbrander gordonbrander left a 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(
Copy link
Contributor

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.

Copy link
Contributor

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" }
Copy link
Contributor

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" }
Copy link
Contributor

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 = {}
Copy link
Contributor

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.

@seefeldb seefeldb closed this Jun 18, 2024
@seefeldb seefeldb deleted the common-ui-inputs branch June 18, 2024 22:12
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