Conversation
|
Hi folks! I've started adding TS types to fields, but I'll need your help to confirm what properties I'm missing, identify common properties that I might be missing as well, what properties and optional/mandatory etc. This is why the PR is still in draft 🙂 EDIT: I'm seeing that the CI is failing, so I guess I'll start with that 😅 |
| @@ -1,47 +1,123 @@ | |||
| import type { JsfSchemaType } from '../types' | |||
There was a problem hiding this comment.
Hi @eng-almeida 👋 I'll review it this afternoon around 16:00!
There was a problem hiding this comment.
@eng-almeida when do you plan to finish this MR?
There was a problem hiding this comment.
Hey @sandrina-p, sorry for dropping the ball here. I'll try to wrap this up next week 🤞
There was a problem hiding this comment.
Hi @eng-almeida! Could you wrap this up this year? 🎁
There was a problem hiding this comment.
Hey @eng-almeida, I added comments in two places regarding non-optional properties but this basically applies to all non-optional properties on those field types.
We don't actually have any sort of validation that checks that when for example 'x-js-presentation': { 'inputType': 'money'} is given there is also 'x-js-presentation': { 'currency': 'EUR'}.
@sandrina-p, is that something we do in v0?
next/src/field/type.ts
Outdated
|
|
||
| export interface FieldTextarea extends BaseField { | ||
| type: 'textarea' | ||
| maxLength: number |
There was a problem hiding this comment.
maxLength ia not mandatory in a JsonSchema and if not provided it won't be in the field.
| maxLength: number | |
| maxLength?: number |
next/src/field/type.ts
Outdated
| export interface FieldOption { | ||
| export interface FieldText extends BaseField { | ||
| type: 'text' | ||
| maxLength: number |
next/src/field/type.ts
Outdated
| inputType: 'statement' | ||
| severity: 'warning' | 'error' | 'info' | ||
| } | ||
| [key: string]: unknown |
There was a problem hiding this comment.
This line hides a lot of TS errors that are hard to address without it. I'll keep investigating...
| description?: string | ||
| fields?: Field[] | ||
| // @deprecated in favor of inputType, | ||
| export type FieldType = 'text' | 'number' | 'select' | 'file' | 'radio' | 'group-array' | 'email' | 'date' | 'checkbox' | 'fieldset' | 'money' | 'country' | 'textarea' |
There was a problem hiding this comment.
todo: Here's my controversial opinion: JSF does not care about FieldType, it's whatever comes from JSON schema x-jsf-presentation.inputType. We do not provide any extra logic/validation just based on it, right? (right? 👀)
So answering @lukad question:
We don't actually have any sort of validation that checks that when for example 'x-js-presentation': { 'inputType': 'money'} is given there is also 'x-js-presentation': { 'currency': 'EUR'}. @sandrina-p, is that something we do in v0?
We don't and we shouldn't. That logic is a concern of Remote internals, not JSF as headless generator/validator. If we ever validate that, it's another layer on top of JSON-SCHEMA-FORM
Does it make sense to you both?
There was a problem hiding this comment.
P.S. I know, this kind affects most of your MR, as the field types should be based on the json schema type, not the inputType 😶
There was a problem hiding this comment.
Hi @eng-almeida , when do you plan to finish this PR? :)
next/src/field/type.ts
Outdated
| export interface FieldOption { | ||
| label: string | ||
| value: string | ||
| description?: string |
There was a problem hiding this comment.
todo:
- The
FieldOptionis a mirror ofoneOf, so if theoneOfhas,fooinside, then the Type includesfootoo. valueis not necessarily a string, can be anything, number, bool, even object. It's a mirror ofoneOf.const🪩valuecan be optional too :p Look at this example withpattern
| export interface FieldRadio extends BaseField { | ||
| type: 'radio' | ||
| options: FieldOption[] | ||
| direction?: 'row' | 'column' |
There was a problem hiding this comment.
todo: Here's another example. This is Remote internal need, not a JSF concern. A few other examples down the line (currency, fileDownload, statement, etc... All of those Types can exist, but not at JSF.
There was a problem hiding this comment.
You're right 💯 ! Probably the right move is to extend these types on Remote SDK project which uses Remote JSON Schemas 👍
next/src/field/type.ts
Outdated
| value: unknown | ||
| [key: string]: unknown | ||
| description: string | ||
| fields: () => Field[] |
There was a problem hiding this comment.
bug: This is v0, in v1 is fields: Field[] (Once #177 is merged)) :D
3 participants
Adds TS types for individual fields.