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
NoelDeMartin
left a comment
There was a problem hiding this comment.
Added a few initial comments, mostly regarding a11y and component architecture.
| @@ -0,0 +1,5 @@ | |||
| declare global { | |||
| interface HTMLElementTagNameMap { | |||
There was a problem hiding this comment.
I like this, but we should generate it automatically at build time instead of doing it by hand. We could create a new Vite plugin that emits this file in vite-config/components.ts.
| rdfTurtleFormatSource=${rdfTurtleFormatSource} | ||
| rdfURI=${rdfURI} | ||
| whichForm=${whichForm} | ||
| rdfName=${rdfName} | ||
| subjectTurtleFormatSource=${subjectTurtleFormatSource} | ||
| subjectName=${subjectName} | ||
| subjectURI=${subjectURI}> |
There was a problem hiding this comment.
Do we need all these attributes? This is a bit confusing, I'm not sure what each property does and why we need so many.
Could it work with something like this?
<solid-ui-rdf-form
form="https://solidos.solidcommunity.net/public/2021/solidUiFormTestData/dummyFormTestFile.ttl#form"
subject="https://solidos.solidcommunity.net/public/2021/alice.ttl#me"
>
</solid-ui-rdf-form>Internally, it could resolve the form and subject from the store associated to the authenticated user.
| @property({ type: String }) | ||
| set rdfURI (value: string) { |
There was a problem hiding this comment.
I think these two propertie (rdfURI and subjectURI) can probably be simplified a lot, I'm not sure we need the _parsedUrl variables or duplicating the URL parsing code. Check out Lit converters.
|
|
||
| render () { | ||
| // TODO: detect format | ||
| loadDocument(store, this.rdfTurtleFormatSource, this.rdfName, this.rdfURI) // load form |
There was a problem hiding this comment.
Instead of using solid-logic's store directly, maybe we can encapsulate this into the Auth interface?
Not sure if we want to couple both concepts though (store and Auth), but maybe we could given that they are already coupled in the solid-logic implementation (I think).
This would also allow us to improve the storybook stories so that they don't require solid-logic. Right now, it seems like we're passing the full RDF graph to the component, but that shouldn't be necessary (I suspect that's a workaround because solid-logic doesn't work in storybook).
Check out how the authentication is configured in storybook and elsewhere, and make sure to learn about Lit context.
| }) | ||
| const me = Namespace(this.subjectURI + '#')(this.whichSubject) | ||
|
|
||
| return html` |
There was a problem hiding this comment.
Other than the inputs, shouldn't this output a native <form> as well? Ideally, whatever is generated from this component ends up being a "real" form. That is important in order to integrate with accessibility tooling and user expectations (implicit form submission, etc.).
| return html` <solid-ui-rdf-input | ||
| .store=${store} | ||
| .formSubject=${sym(part.value)} | ||
| .dataSubject=${me} |
There was a problem hiding this comment.
I think all inputs should also have a name attribute. Otherwise, they'll be ignored by the form (again, important for a11y, etc.).
| @property({ attribute: false }) | ||
| accessor store!: LiveStore |
There was a problem hiding this comment.
Instead of passing the store as a property, I think we should use Lit context instead. See my other comments talking about this.
| const inputValue = this.termToInputValue(selectedTerm, params) | ||
|
|
||
| return html` | ||
| ${inputLabel ? html`<label>${inputLabel}</label>` : ''} |
There was a problem hiding this comment.
This label won't be associated with the input, given that it doesn't have a for attribute. One way to avoid using for is to include both the label text and the inside the label element, like this:
<label>
Name:
<input name="name">
</label>I know it looks counterintuitive, but I'm pretty sure this is correct HTML and I've used it many times :).
In any case, the for attribute is probably the best solution. We can use generateId() from src/lib/components/ids.ts to create a unique id for each input.
| 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 |
2 participants
This is for now: