-
Notifications
You must be signed in to change notification settings - Fork 60
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
StructForm widget for creating modals with a Form #57
base: master
Are you sure you want to change the base?
Conversation
This is pretty cool. Just an idea here, what do you think of changing
I will totally accept the PR with just handling the |
That's a good idea. I'll work on a change. |
Isn't this what |
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 update the top README file and add an example in cmd?
"fyne.io/fyne/v2/widget" | ||
) | ||
|
||
type StructForm struct { |
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 add some documentation for all exported type?
widget.BaseWidget | ||
structType reflect.Type | ||
modal *widget.PopUp | ||
canvas fyne.Canvas |
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.
You do not need to store this value if you only access it from within NewStructForm.
widget/struct_form.go
Outdated
submit func(s interface{}) | ||
} | ||
|
||
func NewStructForm(c fyne.Canvas, s interface{}, submit func(s interface{})) *StructForm { |
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.
It could be useful to split the function in two, one to create just the form and another to pack it in a dialog.
This generate the item of NewForm by using reflection. It could use NewForm internally I guess. |
I think this is confusing widgets and modal dialogs. A widget can be used in a modal dialog - or if you want to make it a modal popup then use dialog for a consistent look. From my cursory glance it seems like this is a powerful widget that should be separated from the modality concept - so it can be used for any location. |
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.
This is looking good, but note you have outstanding comments from @Bluebugs.
Just thinking out loud here, but did you consider extending Form instead of BaseWidget? Perhaps the NeeStructForm could even extend form - I'm not sure but I wonder if that is cleaner? (I think the CreateRenderer could directly return Form.CreateRenderer).
Just trying to catch up on things, where did this get to @Bluebugs @aodhan-domhnaill ? It's such a cool feature, I am embarrassed that we let it sit. |
Sorry. I kind of forgot about this because life stuff. I can try to pick it up again soon |
Example use case,