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

Multi: Refactor MasterPage and PageStack #250

Closed

Conversation

ukane-philemon
Copy link
Collaborator

Closes #249

-- Refactor app.NewMasterPage to accept and handle a start page.
-- Refactor *app.MasterPage.Display()
-- Refactor app.PageStack.Push to only prepare the next page.
-- Refactor pages that embed app.Masterpage.
-- Refactor app.PageStack.Pop()
-- Fix some TODOs.

app/pagestack.go Outdated Show resolved Hide resolved
-- Refactor app.NewMasterPage to accept and handle a start page.
-- Refactor *app.MasterPage.Display()
-- Refactor app.PageStack.Push to only prepare the next page.
-- Refactor pages that embed app.Masterpage.
-- Fix some TODOs.

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@ukane-philemon ukane-philemon marked this pull request as ready for review December 5, 2023 00:14
if pg.tab.SelectedIndex() == 1 {
pg.Display(exchange.NewCreateOrderPage(pg.Load))
}
pg.CurrentPage().HandleUserInteractions()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forwarding User Interaction should happen after a new page has been assigned if we hit the tab changed if branch. Same thing applies to other master pages.

@dreacot
Copy link
Member

dreacot commented Dec 7, 2023

@ukane-philemon i'm not fully sure what the scope of this PR aims to achieve or it's necessity

@ukane-philemon
Copy link
Collaborator Author

@ukane-philemon i'm not fully sure what the scope of this PR aims to achieve or it's necessity

Mainly this "Refactor app.NewMasterPage to accept and handle a start page.". This way we don't have to check if pg.CurrentPage() == nil everywhere when we can initialize with a page.

@JustinBeBoy
Copy link
Collaborator

@ukane-philemon i'm not fully sure what the scope of this PR aims to achieve or it's necessity

Mainly this "Refactor app.NewMasterPage to accept and handle a start page.". This way we don't have to check if pg.CurrentPage() == nil everywhere when we can initialize with a page.

I agree with @dreacot , I'm not sure about the purpose of this PR, your PR and the current PR are not much different.

  • Current code: Whenever we create a new master page, obviously pg.CurrentPage() will be nil, instead of checking we will always create a page for it.
  • Your PR: Add a page when creating the master page but that is optional, it does not prevent pg.CurrentPage() from being nil if the page is not initialized for the master page, If the page passed in is mandatory then there would be no need to check pg.CurrentPage() == nil

This PR I see is just another approach but the problem is still not solved as you said

@ukane-philemon
Copy link
Collaborator Author

Your PR: Add a page when creating the master page but that is optional, it does not prevent pg.CurrentPage() from being nil if the page is not initialized for the master page, If the page passed in is mandatory then there would be no need to check pg.CurrentPage() == nil

Yes, most of the master pages don't need to check as they always have a start page. Only the Window's masterPage and dex masterPage check for a nill CurrentPage() and with #238, we can tell which page to display from the onset for the dex masterPage.

@dreacot dreacot marked this pull request as draft December 11, 2023 19:09
@dreacot
Copy link
Member

dreacot commented Dec 11, 2023

moving to backlog

@ukane-philemon
Copy link
Collaborator Author

I'll resubmit if this is needed in the future.

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.

app.NewMasterPage should accept a start page
4 participants