-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Implement DatetimeControl and add tests #24
Conversation
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.
Can you please add the required
asterisk?
src/testSchemas/datetimeSchema.ts
Outdated
type: "string", | ||
format: "date-time", | ||
scope: "#/properties/The Future is Now", | ||
} as const |
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.
What does as const
achieve? I've also seen a satisfies JSONSchema
pattern, and I opted for that.
9771f6b
to
fcf7d08
Compare
fcf7d08
to
fbece4d
Compare
1485adb
to
0409860
Compare
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.
G2G if you change peer dependencies to regular dependencies--locale configurability can wait TBH
FYI I updated this to be DateControl since this is what it actually is. DatetimeControl and TimeControl can be added later. Following what jsonforms has: https://jsonforms.io/docs/date-time-picker/ |
@DrewHoo @NathanFarmer Could you review this now? All the tests are passing and it's been rebased |
@DrewHoo Looks like I need your review on it too since you previously requested changes. |
Looks like dependencies still aren't updated? Also the DateControlOptions ought to be differentiated instead of added to a union with TextControlOptions. Something like this might work:
|
@DrewHoo Is this no longer required? If not we can close this PR |
Yeah I think so! |
No description provided.