-
Notifications
You must be signed in to change notification settings - Fork 46
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
Split #83
Split #83
Conversation
- added a pages field to Window - changed Window.current to a string representing the current page map key - added string key constants for each page - switched the Wallets page to a self contained page that declares and handles it's own wallets (this page is still in its preliminary stage) - other pages will be switched over in subsequent commits
- added accounts list - added renaming wallet function
- added dialog
ui/wallet_page.go
Outdated
page.dialog.LayoutIfActive(gtx, func() { | ||
common.LayoutWithWallets(gtx, func() { | ||
page.container.Layout(common.gtx, len(wdgs), func(i int) { | ||
wdgs[i]() | ||
}) | ||
}) | ||
}) |
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.
The page layout should be wrapping the dialog layout. Although currently it still produces the intended result, the page layout in the dialog is counter-intuitive.
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 doesn't seem counter-intuitive to me since the dialog is not part of the page but rather a layer above it.
ui/wallet_page.go
Outdated
if page.beginRename.Clicked(gtx) { | ||
page.renaming = true | ||
page.editor.SetText(current.Name) | ||
} | ||
|
||
if page.cancelRename.Clicked(gtx) { | ||
page.renaming = false | ||
} | ||
|
||
if page.rename.Clicked(gtx) { | ||
name := page.editor.Text() | ||
err := common.wallet.RenameWallet(current.ID, name) | ||
if err != nil { | ||
log.Error(err) | ||
} else { | ||
common.info.Wallets[*common.selectedWallet].Name = name | ||
page.renaming = false | ||
} | ||
} | ||
|
||
if page.delete.Clicked(gtx) { | ||
page.dialog.SetDialog(page.dialog.deleteWallet(common)) | ||
} |
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.
The event handling conditions should be in a separate method, the result is a much cleaner code. To make sure every page has an event handling method, you can define an interface which every valid page must implement.
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 actually prefer this to a separate method given that the only requirement a page has to satisfy now is returning a layout.Widget
so adding an interface seems like unnecessary complexity.
* ui: changed Window-Page architecture - added a pages field to Window - changed Window.current to a string representing the current page map key - added string key constants for each page - switched the Wallets page to a self contained page that declares and handles it's own wallets (this page is still in its preliminary stage) - other pages will be switched over in subsequent commits - removed dialogs as well
Resolves
Issue #82
What's new
pages
field to Window for all the pages.and handles it's own widgets (this page is still in its preliminary
stage).