Skip to content
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

Merged
merged 8 commits into from
Apr 25, 2020
Merged

Split #83

merged 8 commits into from
Apr 25, 2020

Conversation

oluwandabira
Copy link
Contributor

@oluwandabira oluwandabira commented Apr 20, 2020

Resolves

Issue #82

What's new

  • Added a pages field to Window for all the pages.
  • 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 widgets (this page is still in its preliminary
    stage).
  • Other pages were left as is but will be switched over in pull requests.
  • Dialogs are controlled by the pages themselves.
  • Error handling for slow wallet operations is not supported yet

@oluwandabira oluwandabira marked this pull request as ready for review April 22, 2020 05:30
oluwandabira and others added 4 commits April 22, 2020 08:49
- 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
Comment on lines 117 to 123
page.dialog.LayoutIfActive(gtx, func() {
common.LayoutWithWallets(gtx, func() {
page.container.Layout(common.gtx, len(wdgs), func(i int) {
wdgs[i]()
})
})
})
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 125 to 147
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))
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@oluwandabira oluwandabira merged commit d1e77cb into planetdecred:master Apr 25, 2020
song50119 pushed a commit to song50119/godcr that referenced this pull request Apr 24, 2022
* 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
This pull request was closed.
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.

2 participants