Initial RDF forms component#798
Conversation
Prompt: reading the RDFinput file, pls make suggestions of how to impprve the code to make it easier to follow and read. I beliebe it is difficult to follow the fact that one has a rdf forms subject and a data subject as well and how it is all itertwinded. Co-Authored-By: GitHub Copilot (raptor-mini) <copilot@github.com>
NoelDeMartin
left a comment
There was a problem hiding this comment.
Added a few initial comments, mostly regarding a11y and component architecture.
| export type FieldParamsObject = { | ||
| size?: number, // input element size attribute | ||
| type?: InputType, // input element type attribute. Default: 'text' (not for Comment and Heading) | ||
| element?: string, // element type to use (Comment and Heading only) | ||
| style?: string, // style to use | ||
| dt?: string, // xsd data type for the RDF Literal corresponding to the field value. Default: xsd:string | ||
| defaultInputValue?: string, // e.g. 'mailto:'. Default value in input field, will be removed when displaying actual value to user. | ||
| namedNode?: boolean, // if true, field value corresponds to the URI of an RDF NamedNode. Overrides dt and defaultInputValue. | ||
| pattern?: RegExp // for client-side input validation; field will go red if violated, green if ok | ||
| } |
There was a problem hiding this comment.
Many of these are currently unused, right? I see that this is a WIP, so it's fine. But before merging, I would remove everything that isn't used (you know, YAGNI, removing dead code, etc.).
Also, I'm not sure I like the style config... that goes a bit against the idea of RDF forms, given that different UI libraries may render forms differently, and inline styles are probably not going to work correctly in all of them. But if that was a design decision when creating the RDF forms, ok.
Prompt: can this #sym:HTMLElementTagNameMap be generated automatically from vite-config/ components.ts for all RDF suffixed components? Co-Authored-By: GitHub Copilot (raptor-mini) <copilot@github.com>
|
RDFInput will make use of the new Input components when it is merged: #815 |
|
We need to improve the name property of the generated form and make sure they are unique. All HTML elements created automatically from the ontology ui file need names but there could be more input fields called input-name. I am not sure I can just use a random id generator. |
Prompt: I have this accessors rdfURI and subjectURi which should be URIs. I would like to use a lit converter for them instead of my code: https://lit.dev/docs/components/properties/#conversion-converter Help me with it. Prompt: I want the converter to only return URL or null Co-Authored-By: GitHub Copilot (raptor-mini) <copilot@github.com>
|
I just saw this now, here's my thoughts:
It depends on how the form is going to be used by the consumer. If everything is handled in javascript, there's nothing wrong with using names generated by the id generator. However, if the consumer wants to actually submit this form to a server they would need to know the actual names. I haven't looked at the Basically, the behaviour I would like to see is that by default it generates random input names using the id generator, but if the RDF definitions specify the name of the form control, we'll use that. If those are not unique, I don't think we should do anything about it, that's the responsibility of whoever is defining the RDF (if they want two fields to have the same name, let them do it, although we could throw a helpful warning in the console or something). That would be useful for different reasons, not just to handle the form submission. For example, the name of an input can influence some browser extensions or autocompletes. |
|
What we also need to decide is if we take the input size property into consideration. |
|
I guess this is something to discuss, but personally I don't think the RDF should be able to affect the UI (things like size). That's the whole point of the approach, that the form gets rendered using the RDF definition in a way that works for the current UI/project. I guess it's fine to have some properties like "primary color" for theming or something like that. But even then, shouldn't the "primary color" be set by the application where the form is rendered in? Maybe it can be useful to change an accent or something, to distinguish a button or input from others... But I'm not sure I like the idea. As per how the size is determined now, I haven't looked too much into it, but usually inputs have a min-width or take 100% of their container. That's something that should be configured in the form. For now I think it's fine to simply have a max-width in the form and otherwise take up the entire container. We can tweak these things later, possibly with some properties in the component. |
There was a problem hiding this comment.
Pull request overview
Introduces an initial RDF Forms implementation for the component library, wiring it into Storybook and adding an RDF-backed input Web Component, alongside supporting infrastructure (store context, helpers, and generated custom-elements typing).
Changes:
- Add
<solid-ui-rdf-form>and<solid-ui-rdf-input>components plus Storybook story/provider/store plumbing for rdflib-backed document loading. - Add rdflib form helper utilities and field parameter mappings for selecting appropriate HTML input types.
- Add a Vite plugin to auto-generate
src/types/custom-elements.d.ts, and extend existing form controls with areadonlyAPI surface.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.mts | Registers a new Vite plugin to generate custom element typings. |
| vite-config/components.ts | Implements custom element type generation and a Vite plugin hook for it. |
| src/types/custom-elements.d.ts | Updates the (now-generated) HTMLElementTagNameMap entries for custom elements. |
| src/storybook/store/StorybookStore.ts | Adds a Storybook rdflib store wrapper for RDF forms. |
| src/storybook/components/StorybookProvider.ts | Provides the RDF store context in Storybook. |
| src/lib/forms/store/StoreContext.ts | Introduces a Lit context interface for injecting an rdflib LiveStore. |
| src/lib/forms/store/NoopStore.ts | Adds a default “no store” implementation that throws when accessed. |
| src/lib/forms/rdfFormsHelper.ts | Adds document loading, fetching, and RDF form utility helpers. |
| src/lib/forms/fieldParams.ts | Adds field-type → HTML input parameter mappings for RDF form fields. |
| src/lib/components/traits/InputTrait.ts | Extends the input trait target contract to include readonly. |
| src/components/select/Select.ts | Adds a readonly property to <solid-ui-select> and attempts to bind it to the native <select>. |
| src/components/rdf-input/RDFInput.ts | Implements <solid-ui-rdf-input> for reading/writing RDF-backed field values. |
| src/components/rdf-input/index.ts | Exports the RDF input component entrypoint. |
| src/components/rdf-form/RDForm.stories.ts | Adds a Storybook story for <solid-ui-rdf-form>. |
| src/components/rdf-form/RDFForm.ts | Implements <solid-ui-rdf-form> for rendering form parts from RDF. |
| src/components/rdf-form/index.ts | Exports the RDF form component entrypoint. |
| src/components/input/Input.ts | Adds readonly support to <solid-ui-input> and binds it to the native <input>. |
| src/components/input/Input.styles.css | Adds read-only styling rules for the native <input>. |
| src/components/combobox/Combobox.ts | Adds readonly support to <solid-ui-combobox> and binds it to the native <input>. |
| .gitignore | Ignores the generated src/types/custom-elements.d.ts file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private async updateData (e: CustomEvent) { | ||
| const newValue = (e.target as HTMLInputElement).value | ||
| this._pendingUpdateValue = newValue |
I have decided to do a generateId (random id) since we did not have the name attribute at all in the past. A random one is enough for now. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…istry@3.1.2-2) (latest: rdflib@2.4.0)
…istry@3.1.2-2) (latest: rdflib@2.4.0)
NoelDeMartin
left a comment
There was a problem hiding this comment.
I spotted a couple of things that can be improved (some actual problems, like the generateId() in the render), but otherwise I think this is pretty close to being ready.
| return html` | ||
| <solid-ui-input | ||
| label="${inputLabel}" | ||
| name="name-${generateId()}" |
There was a problem hiding this comment.
The generateId() helper shouldn't be called inside of render, if we do we'll get a different ID every time the component re-renders. Instead, we should set it on a private property in the constructor.
Actually, I see this component is not using the FormControl trait. I wonder if it should? If it does, that trait already generates an internal id for each control, so we'll be able to use that one to get the name.
| private accessor storeContext: StoreContext = DEFAULT_STORE | ||
|
|
||
| @property({ attribute: false, type: Object }) | ||
| accessor formSubject!: NamedNode |
There was a problem hiding this comment.
| accessor formSubject!: NamedNode | |
| accessor formSubject: NamedNode | null = null |
It's generally an anti-pattern to use ! in type declarations. That assumes that the component will always be used with the "required" attributes, but what if someone uses it without them? I'd rather we control this use-case in the code to throw an appropriate error message, rather than something like "trying to call .something in undefined". We can encapsulate reading this property in a getter, so it shouldn't be much of a change.
Same feedback to other attributes in this and other components.
| private accessor entireDataIsReadonly: boolean = true // to protect data, we default to not editable | ||
|
|
||
| @state() | ||
| private accessor _loadVersion = 0 |
There was a problem hiding this comment.
I'm not sure what this and the storeVersion proeprties are for, can you help me understand?
| } | ||
|
|
||
| @state() | ||
| private accessor entireDataIsReadonly: boolean = true // to protect data, we default to not editable |
There was a problem hiding this comment.
This seems to be used only in the render() method, I think it'd be better to keep it as a function-scoped variable rather than component state.
| ` | ||
| } | ||
|
|
||
| protected updated (changedProperties: Map<PropertyKey, unknown>) { |
There was a problem hiding this comment.
I think this should override willUpdate rather than update. It shouldn't make much of a difference, but it's more appropriate.
| .storeVersion=${this._loadVersion} | ||
| .readonly=${this.entireDataIsReadonly} | ||
| ></solid-ui-rdf-input> | ||
| <br>` |
There was a problem hiding this comment.
I don't think this <br> should be necessary, we should deal with these in CSS.
| id="${this.inputTrait.inputId}" | ||
| name=${this.name} | ||
| ?required=${this.required} | ||
| ?disabled=${this.readonly} |
There was a problem hiding this comment.
I think native selects also have a readonly attribute, so maybe we should use that one instead of disabled.
| if (this.store) { | ||
| // read `store` so the property is considered used | ||
| } |
There was a problem hiding this comment.
Was this added to silence a typescript or linting error? If that's the case, I think it'd be better to add an ignore-comment rather than add some runtime behaviour.
| node_modules | ||
| dist | ||
| src/versionInfo.ts | ||
| src/types/custom-elements.d.ts |
There was a problem hiding this comment.
Since this was added to the gitignore (which I like!), we should remove the file from the repository as well.
| const lines = [ | ||
| '/**', | ||
| ' * This file is auto-generated by vite-config/components.ts.', | ||
| ' * Do not edit this file directly.', | ||
| ' */', | ||
| '', | ||
| ] |
There was a problem hiding this comment.
I don't think this is necessary anymore, given that this generated file won't be tracked in the repository anymore.
This is for now: