Skip to content

Conversation

@Rathoz
Copy link
Collaborator

@Rathoz Rathoz commented Aug 5, 2025

Summary

Idea on improving the the Widget handling when it comes to props, default props, required props and coming from wikicode world where everything in a string.

How did you test this change?

Comment on lines +26 to +28
if Table.isNotEmpty(self.defaultProps) then
return self.defaultProps
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo add a comment that this handling is only legacy and deprecated

Comment on lines +30 to +31
if prop.type == 'integer' then
args[propName] = WidgetFactory._parseInteger(args[propName])
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about non integer numbers?
maybe

Suggested change
if prop.type == 'integer' then
args[propName] = WidgetFactory._parseInteger(args[propName])
if prop.type == 'integer' or prop.type == 'number' then
args[propName] = WidgetFactory._parseNumber(args[propName])

Comment on lines -14 to +16
---@param args {widget: string, children: ((Widget|Html|string|number)[])|Widget|Html|string|number, [any]:any}
---@param args {widget: string, children: any, [any]:any}
Copy link
Collaborator

@ElectricalBoy ElectricalBoy Sep 6, 2025

Choose a reason for hiding this comment

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

imo make a type alias for the original children type and use that
so something like Renderable = Widget|Html|string|number and make children of type Renderable|Renderable[]

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice idea we could use it in some other places too :)

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.

4 participants