-
Notifications
You must be signed in to change notification settings - Fork 0
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
SOF-6794: add more rjsf widgets #29
Conversation
|
||
type InfoPopoverOptions = InfoPopoverProps & { content: string }; | ||
|
||
export default function InputWithInfoPopover(props: WidgetProps) { |
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 wonder if we should mark somewhere what role the components fulfill (e.g. Template, Widget, or Field). If we plan to add a couple of more RJSF components, then perhaps we can divide them into subfolders named templates, widgets, and fields. As we have only 4 right now, perhaps a simple JSDoc string would suffice. What do you think?
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.
Agree, dividing into subfolders make a clean separation.
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.
looks good 👍
|
||
export default function InputWithInfoPopover(props: WidgetProps) { | ||
const { uiSchema } = props; | ||
const infoPopover = (uiSchema ? uiSchema["ui:options"]?.infoPopover : {}) as InfoPopoverOptions; |
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.
That's a very strange type casting here.
And later you use it with conditions like infoPopover?.title
as it can be null or undefined
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.
Typed it more explicitly
|
||
const { BaseInputTemplate } = Templates; | ||
|
||
type InfoPopoverOptions = InfoPopoverProps & { content: string }; |
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 found you defined this type three times in this PR.
Why is that needed?
No description provided.