-
Notifications
You must be signed in to change notification settings - Fork 50
Introducing Reusable Setting Views #197
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
Conversation
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.
ACK 3e62f27
Much clearer definition of the Settings pages and much better file naming when looking for a page with specific context. One minor quirk with the naming of the page and the selection component that can be thought about and addressed later if a solution is found.
Views render as expected and no qml warnings when run.
detailActive: true | ||
detailItem: ColumnLayout { | ||
spacing: 0 | ||
StorageSettings { |
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 naming here ends up being a bit funny as its nearly identical to the parent component's name. Maybe append to the component name something that signifies that it is just the selection piece or use "import as" when bringing in the component.
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.
Just noticed the naming here are the same words in reversed order :D, can anyone suggest better naming here?
@GBKS: SettingsStorage.qml
represents the actualy page view for the Settings that are related to Storage. And StorageSettings
is a component we place inside of SettingsStorage.qml
that represents the table of advanced settings which are related to Storage.
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.
since this component is already under a "settings" folder. Maybe StorageSettingsPage is better name for the component and then something equivalent for the other components since it is a "Page" that holds the "StorageSettings"
Loader { | ||
source:"onboarding01a.qml" | ||
InformationPage { | ||
Layout.fillWidth: true |
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.
SwipeViews don't handle Layout properties. Think this can be removed.
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.
thanks for spotting these, it's a bad habit of mine to place this until the view is working correctly. Since you've already opened a PR to address them, i've left this as is.
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.
Noticed some Layout properties that likely are unused. Might be best to address in a separate PR.
Edit: Starting PR #201
Page { | ||
background: null | ||
clip: true | ||
InformationPage { | ||
Layout.fillWidth: true |
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 Layout property can be removed
Page { | ||
background: null | ||
clip: true | ||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
@@ -9,31 +9,26 @@ import org.bitcoincore.qt 1.0 | |||
import "../../controls" | |||
import "../../components" | |||
|
|||
Page { | |||
background: null | |||
InformationPage { | |||
Layout.fillWidth: true |
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.
Remove
Loader { | ||
source:"onboarding05a.qml" | ||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
Loader { | ||
source:"onboarding06a.qml" | ||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
import "../../components" | ||
|
||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
|
||
InformationPage { | ||
background: null | ||
Layout.fillWidth: true |
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.
Remove
import "../../components" | ||
|
||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
import "../../components" | ||
|
||
InformationPage { | ||
Layout.fillWidth: true |
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.
Remove
This extracts out the navbar element defintions from the settings page itself, and moves it to where the page is instantiated. This allows the settings views to be reused anywhere else we want to use them, and are now no longer limited to being used within the onboarding wizard.
3e62f27
to
30db8cc
Compare
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.
ACK 30db8cc, I have reviewed the code and it looks OK, I agree it can be merged.
Github-Pull: bitcoin-core#197 Rebased-From: 5f11241
Github-Pull: bitcoin-core#197 Rebased-From: 86170be
This extracts out the navbar element defintions from the settings page itself, and moves it to where the page is instantiated. This allows the settings views to be reused anywhere else we want to use them, and are now no longer limited to being used within the onboarding wizard. Github-Pull: bitcoin-core#197 Rebased-From: 30db8cc
Github-Pull: bitcoin-core#197 Rebased-From: 5f11241
Github-Pull: bitcoin-core#197 Rebased-From: 86170be
This extracts out the navbar element defintions from the settings page itself, and moves it to where the page is instantiated. This allows the settings views to be reused anywhere else we want to use them, and are now no longer limited to being used within the onboarding wizard. Github-Pull: bitcoin-core#197 Rebased-From: 30db8cc
Github-Pull: bitcoin-core#197 Rebased-From: 5f11241
Github-Pull: bitcoin-core#197 Rebased-From: 86170be
This extracts out the navbar element defintions from the settings page itself, and moves it to where the page is instantiated. This allows the settings views to be reused anywhere else we want to use them, and are now no longer limited to being used within the onboarding wizard. Github-Pull: bitcoin-core#197 Rebased-From: 30db8cc
This makes the settings views independent from the onboarding views and refactors the definition of the settings views so that these now independent settings view pages can be reused anywhere and as many times as we need to; they are not delegated to only be used within the onboarding wizard.
Desktop
Master
PR