-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix Datalink inputs #11376
base: develop
Are you sure you want to change the base?
Fix Datalink inputs #11376
Conversation
i think this is out of scope unfortunately. the way the event handler currently works, we cannot |
+1 to that - it seems that I can create a data link without hostname or database name - but these 2 fields are required. Such datalink gets created but then when I |
@@ -186,7 +200,7 @@ export default function JSONSchemaInput(props: JSONSchemaInputProps) { | |||
> | |||
{propertyDefinitions.map((definition) => { | |||
const { key, schema: childSchema } = definition | |||
const isOptional = !requiredProperties.includes(key) | |||
const isOptional = !requiredProperties.includes(key) || isAbsent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be too simple for the fix we need.
Fields that are themselves required, should still be marked red and not accepted if they are absent.
Only required sub-fields whose parent field is optional, do not have to be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this is a bit of a misnomer - isAbsent
means that the parent field is optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foo (object field, optional, currently absent, isAbsent = false)
Bar (isAbsent = true, not optional)
Baz (isAbsent = true, not optional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this is a bit of a misnomer -
isAbsent
means that the parent field is optional
Oh... Yeah so that name is a bit misleading indeed
Hmm, I see. It may be that the Postgres always allowed empty string for hostname/database name. So the fields are not actually missing, they are just I guess we may want to change the schema to require at least |
Pull Request Description
Autocomplete
dropdown automatically whenautoFocus
is nottrue
Important Notes
None
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.or the Snowflake database integration, a run of the Extra Tests has been scheduled.