Skip to content
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

[chore] Add rows property to field schema #407

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kgrhartlage
Copy link
Member

Description

We already added support for this in the server repo (PR).

I also tried to narrow down the supported combinations of different schema settings.

@kgrhartlage kgrhartlage self-assigned this Oct 4, 2023
Comment on lines 5 to 8
not:
oneOf:
# only allow `ui:options.rows` for string fields
- required: ['type', 'ui:options']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it applied as a negative logic?? how would it looks like on the rendered documentation schema?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How else can we make this work? This, for example, is what we do want to support:

type:
  const: boolean
ui:widget:
  type: string
  enum: ['checkbox','switch']

If we nest this in oneOf we also need to add a case for all the possible valid combinations for a boolean field, which cannot include in this combination of settings though, must have ui:widget as optional, and must support the other custom properties and the Draft 07 ones. 🤔

If we nest this in allOf... well then I don't see how we can exclude anything

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored it, we are now left with just one not case. This will require changes on the schema docs, since we have not used the if and then keywords before.

@@ -67,6 +72,55 @@ allOf:
- $ref: '#/definitions/field-schema/relationship'
- $ref: '#/definitions/field-schema/script'
- $ref: '#/definitions/field-schema/timer'
# only allow text & textarea widgets for string fields
- if:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we would add a description to express the constraint the schema implements. I would likely lean to only adding it to if and not split it up between if and then, since they both belong together anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks much better 👍

Interesting though that you put the type inside then and the configuration inside if.. I would think that it make more sense to do it the other way around, but not sure if it works, e.g.

if:
  type: string
then:
  ui:widget:
    enum: ['text', 'textarea']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can flip this

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.

2 participants