Introduce Signal Based Navigation Model to have Self-Contained Pages #427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pages exist within a view, in this page <-> view dependency, somewhere
inside of there, at least for views that are 'flows' or 'wizards,' there
needs to be logic on:
a) when to move to a new page
b) what page to move to.
We are currently handling this navigation logic primarily inside the
definition of a page itself. Inside of the page, we assume a hard-coded
ID for a parent view and call navigation function directly to the view
from within the page itself telling it to:
a) to pop or push
b) the id of the page we want to push.
This is bad because:
1. It creates a dependency between page declarations that does not
need to exist. As in, PageB.qml doesn't really exist unless
PageA.qml includes code to go to it.
2. This prevents us from being able to 'statically' define and
clearly contain pages. Clearly containing pages makes it so that
when we want to reason about PageA.qml, we only have to look
inside of PageA.qml and the components that are used within that
file; we don't have to go on a chase for interlinked dependencies
on id's or properties that are hidden or at least not easy to
find in other pages.
3. We should never assume outside id's from within a page, as these
can change, and when they change, the page that assumes said
outside id is now broken, and debugging can be annoying.
All of this is unnecessary.
In this commit, we address the issue of the logic for navigation being
contained within the page definitions. This proposes a new navigation
model based on signals and first applies it to the onboarding flow.
Pages within our onboarding flow now:
1. Emit a signal when it's time to go to a new page
2. The parent View itself handles the signals and contains the logic
for navigation.
This removes the dependence on having a hardcoded outside ID within a
a page, and is one step to moving us to pages with no dependency on
other pages.
Here, pages are clearly contained, but the logic of moving to SettingsDeveloper from within SettingsAbout and SettingsProxy from within SettingsConnection is still contained here within the definitions of SettingsAbout and SettingsConnection. While all pages are independently still usable, we do want to iron out this one kink in a follow-up.
Makes our CreateWallet flow into a wizard, as that is what it should be. The CreateWalletWizard is introduced as a StackView with the associated pages implementing signal based navigation. Previously, there was an interlinked dependency between CreateName and CreatePassword in that CreateName would pass a string for walletName to CreatePassword. This dependency is removed by having CreateName set the string in a property contained in the CreateWalletWizard StackView, and CreatePassword pulling in this property to satisfy its requirement for walletName. This is a temporary workaround so that we can still have clearly contained pages here. A follow-up should move this into a more appropriate backend object
MarnixCroes
reviewed
Oct 25, 2024
| onNext: aboutSwipe.incrementCurrentIndex() | ||
| } | ||
|
|
||
| states : [ |
There was a problem hiding this comment.
nit
Suggested change
| states : [ | |
| states: [ |
There was a problem hiding this comment.
we should set a style guide actually
|
Concept ACK No qml errors when going through onboard. Looks like there are just some whitespace issues to fix to make the lint job happy. |
In containing pages, everything a page needs should be defined where the page is declared. Allowing to alias nav elements into a page breaks the idea of pages being statically defined. The action of aliasing down nav elements means that the real declaration of a page is not in its 'filename.qml', but instead where the page is used, meaning a page can be defined an infinite amount of times in an infinite amount of ways; we don't want this. The only reason that we were aliasing down nav elements is that we have reusable pages for settings and what elements the nav bar will contain is different depending on whether we are onboarding or not. So, we should treat that as the navbar for the page has two states, so we should have logic for these states instead of defining the page in different locations in different ways. This state logic is handled by setting a property of onboarding or not, a follow-up can potentially move this into an appropriate backend object such as AppMode.
Renaming for consistency with other pages that 'back' signal and handles the 'back' signal. No reason to have some pages call this differently.
jarolrod
force-pushed
the
contain-pages
branch
from
59aeba2
to
3f94240
Compare
johnny9
approved these changes
Oct 27, 2024
There was a problem hiding this comment.
post-merge tACK 3f94240
we should set a style guide actually
As discussed offline, we should do that + some specific dev notes for this repo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here we introduce a Signal Based navigation model, so that we can have self-contained pages. Self-contained pages include everything they need where they are declared, and do not have more than one definition. This makes pages modular, easier to reason about, but also easier to maintain by eliminating dependencies on hard-coded IDs, direct calls to parent views, and interlinked properties between pages.
When we want to reason about
PageA.qml, we only have to look in there and the child components it uses, and not have to use a search function to ensure we have the entire scope ofPageA.qml.To Achieve this, this PR implements the following main ideas:
Additionally, while here we structure the CreateWallet flow into a Wizard.
Follow-Ups:
Consider moving onboarding and walletName into more appropriate backend objects.