-
Notifications
You must be signed in to change notification settings - Fork 43
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
Onboarding views #124
Onboarding views #124
Conversation
Concept ACK. This PR definitely requires designers' reviewing :) |
Adjust layouts according to https://bitcoindesign.github.io/Bitcoin-Core-App/layout/? |
The centered text in the onboarding sequence isnt ideal - left justified is better. Experimenting with where the text breaks to avoid dangling words such as "a" "the" "in" etc is also better responsive design/text flow. |
I'm not sure what I am doing wrong here trying to test? |
This might be a basic suggestion, but have you installed the QML-specific dependencies? I am mentioning this because I recently made a similar mistake :) In case you have installed the dependencies, it would be best if you could run:
And share the resultant log part relevant to Qt and QML. The relevant part should look something like this: |
Yes, the binary has the same name.
On Sat, 4 Jun 2022 at 00:44, Robert Spigler ***@***.***> wrote:
I'm not sure what I am doing wrong here trying to test?
Installed qml but still only seeing bitcoin-qt (no qml binary). Running
it still results in the old setup.
—
Reply to this email directly, view it on GitHub
<#124 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH3PXPXGMSORHDT3UV66FY3VNKDEBANCNFSM5XPAR2AA>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
…--
Hennadii Stepanov
|
Ah ok thank you both. I was looking for the dependencies here: https://github.com/bitcoin-core/gui-qml/blob/main/doc/dependencies.md (which had no change from bitcoin/bitcoin). Was wondering where the documentation was. Will update in a moment |
Notes: On the 'Info' Page (About; Bitcoin Core is an Open Source Project...):
Strengthen Bitcoin:
Block Clock:
Storage Location:
Storage: Connection:
Other: Possible other config options to add:
Dbcache |
I'm also interested in how we will set up the Networks page: cjdns/i2p/tor/ and/or clear (also incoming/outgoing). |
Thank you very much, @Rspigler, for your thorough review and analysis of the PR. Let me try to address your observations as best as I can.
That is due to improper layouting. I am working on fixing the layouts according to design guidelines, which will fix this issue.
Yes, that's a known limitation. I displayed them as plain text for this PR's initial version to finish up a working prototype of onboarding views. This is intended to fix in the next update of this PR.
Yes. Though this is out of the scope of this PR, this is something we intend to accomplish.
Thanks for pointing that out! This was something I missed while writing the description.
We indeed to keep this page the same as displayed through this PR. But it seems like you have some nice ideas to improve this page for the Desktop. If so, I would love to hear them.
I want to think more about this. Would you please share your reasoning for doing this change?
This is something I would definitely agree with. I myself got confused a lot of times while testing this PR. an anecdote of confusion:I got to a detailed setting page, pressed the back button, pressed the next, and could not understand why the done button was showing up instead of next.
Let us give a thought about it. Thank you very much again for your valuable feedback and thorough analysis. Your review has given me a lot of nice things to work upon :) |
Hi @shaavan, thank you for your response!
The current page has 3 settings:
Bitcoin/bitcoin has documentation on how to manage traffic for Bitcoin Core: (https://github.com/bitcoin/bitcoin/blob/master/doc/reduce-traffic.md). These are
Listening and Blocks Only already discussed above. For Networks, I think this is also a very important setting, for both privacy reasons and also anti-network partition strength. Currently, network settings can be very difficult to set up for beginners. I can see something like this:
For example, doing nothing and being behind a firewall would have Not all combinations are able to exist, but I think it is a nice way to visualize. (Would also include i2p and cjdns) |
As I already mentioned during the recent design call, it would be nice to consider the current content of the views as mere placeholders in this PR context, and focus on workflows, transitions, layouts etc. |
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.
Concept ACK, still reviewing but wanted to talk about the PageIndicator
which is still at the bottom. It was forgotten in #106 to remove the PageIndicator
at the bottom. It is no longer to be used there as per discussions had on the design calls and per the design specifications. The screenshots in the PR description crop this out for some reason.
On PR branch, PageIndicator at bottom
You should just delete the footer of the Wizard
control in a new commit, I have done this here: https://github.com/jarolrod/gui-qml/tree/remove-pageindicator-wizard
Feel free to cherry-pick this commit and then rebase the commits included in this PR on top of that.
@Rspigler Thanks for your interest in the project! Right now we're at a stage where we're figuring out processes and in getting an initial app together. There are some elements that will appear out of place, but all of this will be sorted out fairly soon! |
Updated from e8a6a8d to fc6f6ea (pr124.01 → pr124.02, diff) Addressed @hebasto, @jarolrod, @Rspigler, and @RandyMcMillan comments Changes:
|
tACK fc6f6ea |
fc6f6ea
to
f5a1127
Compare
f5a1127
to
5502d02
Compare
Updated from f5a1127 to 5502d02 (pr124.03 -> pr124.04, diff) Changes:
Visual differences:
|
5502d02
to
a655954
Compare
tACK a655954 |
- This corresponds to the second page, which talks about Strengthening Bitcoin Co-authored-by: jarolrod <[email protected]>
- The page correponds to the block clock page in design file. - The block clock is temporarily replaced with the Bitcoin core app icon
- This page corresponds to the "Storage location" page in design file. - This commit also adds a component file: StorageLocations.qml to facilitate the working of the onboarding04.qml file. Co-authored-by: jarolrod <[email protected]>
- This includes the onboarding05.qml file and it subpages: - onboarding05a.qml : "Storage" - onboarding05b.qml : "Storage settings" - This commit also includes a components file to facilitate the onboarding05b.qml file Co-authored-by: jarolrod <[email protected]>
- This includes the main onboarding06.qml file along with its subpages: - onboarding06a.qml : "Connection" - onboarding06b.qml : "Connection settings" - This commit also updates the ConnectionSettings.qml component to better reflect the design file. Co-authored-by: jarolrod <[email protected]>
Co-authored-by: jarolrod <[email protected]>
802e682
to
5118be4
Compare
Updated from 802e682 to 5118be4 (pr124.08 -> pr124.09, diff) Addessed @GBKS suggestions. Changes:
Some Minor Changes:
Here are the screenshots for all the visual differences after this update:
First of all, thank you very much, @GBKS, for all your valuable suggestions. All these suggestions helped improve the PR a lot.
Since I wanted to be sure of the correct method of importing svg, and png format in resources, I decided to keep this change on hold. Again thank you very much for all your suggestions! A friendly ping to my fellow contributors: @hebasto @jarolrod @mouxdesign @Rspigler @RandyMcMillan |
Will test shortly |
tACK commit 5118be4 I can confirm that the text size changes from pr124.06->pr124.07 has taken place All tests pass |
Great update, @shaavan. Just tested, and noticed a few more things we can address, whether in this PR or elsewhere:
Was also wondering if we need more, and/or more visual, hover/selected states. When hovering a web link on "About", for example, I see the text select cursor. Should probably be the hand cursor, and we could change the text color (to orange) for clearer feedback. |
Layout.alignment: Qt.AlignRight | Qt.AlignHCenter | ||
active: root.description.length > 0 | ||
visible: active | ||
sourceComponent: TextEdit { |
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 QML TextEdit component is not an appropriate component for this. Per it's description: "Displays multiple lines of editable formatted text." The links we want to display are definitely not editable text.
We should just use Text. That way we don't need to hack around TextEdit
by setting it as readOnly
.
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 TextEdit
Component is needed for the options like Storage Limit
or Connection Limit
, which required the values to be editable.
Getting rid of the readonly
property would mean creating separate loaders for editable and non-editable text.
I would like to hear your take on this and would be glad to know a more efficient way to deal with this problem.
@@ -26,6 +26,7 @@ Control { | |||
} | |||
OptionSwitch { | |||
Layout.alignment: Qt.AlignRight | |||
Layout.rightMargin: -12 |
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's not clear to me why we are hardcoding a rightMargin
value of 12
here. This doesn't seem like the correct way to do this, what is the design spec that inspired this change?
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 rightMargin
was explicitly set to correctly position the OptionsSwitch
, to be precisely above the right end of the divider line.
Before | After |
---|---|
![]() |
![]() |
I experimented with other conventional methods of aligning elements using Layouts
and Anchors
before going with hardcoded margin. Still, none of them worked to position the Switch over the divider properly.
import QtQuick.Controls 2.15 | ||
import QtQuick.Layouts 1.15 | ||
|
||
Control { |
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.
As the description of a17b84e states, "This is an object similar to Setting.qml". I think that instead of having this control we can just refactor Setting.qml
to load the desired component in place of the currently hardcoded switch. This adds flexibility to the setting control to fit in with all the use cases required of it within options handling without introducing a new component here, and other components when a new component is desired in the switch's position.
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 would be the ideal way to move forward, and this was something I tried to do while beginning to write this PR.
However, the crucial problem I faced while going with this idea at the beginning was that QML is a declarative language and hence doesn't support conditional statements that are more complicated than a ternary operator.
So it was quite hard to figure out a way to make Settings.qml
switch the type of description based on the option type without using conditional statements. So I went forward by creating Information.qml
from scratch and, one by one, incorporated changes to work with all the option types.
But now, since the Information.qml
has been laid out, and almost all the permutations of various options have been accounted for, we can think of ways to merge these two files effectively.
@@ -27,11 +28,11 @@ Control { | |||
topPadding: root.headerMargin | |||
font.family: "Inter" | |||
font.styleName: root.bold ? "Semi Bold" : "Regular" | |||
font.pointSize: root.headerSize | |||
font.pixelSize: root.headerSize |
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.
in 8763690, you didn't update the PR description to state that we are now using pixelSize
instead of pointSize
when you updated the PR. This is a pretty important change to document. I would suggest extracting out this part into it's own commit (adding a nice description to document the historical reason):
qml: use font pixelSize instead of pointSize
diff --git a/src/qml/controls/ContinueButton.qml b/src/qml/controls/ContinueButton.qml
index 2b98f8baf..d478ba45e 100644
--- a/src/qml/controls/ContinueButton.qml
+++ b/src/qml/controls/ContinueButton.qml
@@ -8,7 +8,7 @@ import QtQuick.Controls 2.15
Button {
font.family: "Inter"
font.styleName: "Semi Bold"
- font.pointSize: 18
+ font.pixelSize: 18
contentItem: Text {
text: parent.text
font: parent.font
diff --git a/src/qml/controls/Header.qml b/src/qml/controls/Header.qml
index 13d42e1c4..59fb01671 100644
--- a/src/qml/controls/Header.qml
+++ b/src/qml/controls/Header.qml
@@ -27,7 +27,7 @@ Control {
topPadding: root.headerMargin
font.family: "Inter"
font.styleName: root.bold ? "Semi Bold" : "Regular"
- font.pointSize: root.headerSize
+ font.pixelSize: root.headerSize
color: Theme.color.neutral9
text: root.header
horizontalAlignment: center ? Text.AlignHCenter : Text.AlignLeft
@@ -42,7 +42,7 @@ Control {
topPadding: root.descriptionMargin
font.family: "Inter"
font.styleName: "Regular"
- font.pointSize: root.descriptionSize
+ font.pixelSize: root.descriptionSize
color: Theme.color.neutral8
text: root.description
horizontalAlignment: root.center ? Text.AlignHCenter : Text.AlignLeft
diff --git a/src/qml/controls/TextButton.qml b/src/qml/controls/TextButton.qml
index 1063a275b..2dce44670 100644
--- a/src/qml/controls/TextButton.qml
+++ b/src/qml/controls/TextButton.qml
@@ -11,7 +11,7 @@ Button {
property string textColor: Theme.color.neutral9
font.family: "Inter"
font.styleName: "Semi Bold"
- font.pointSize: root.textSize
+ font.pixelSize: root.textSize
contentItem: Text {
text: root.text
font: root.font
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 agree with your suggestion regarding splitting the commits and rewriting the commit message. And hence I tried to incorporate them.
However, after merging this PR, I can't figure out a way to do them.
Could you please suggest a way to incorporate this and the following two suggestions? Thanks!
@@ -11,18 +11,6 @@ Page { | |||
property var views | |||
property alias finished: swipeView.finished | |||
background: null | |||
header: RowLayout { |
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.
in 79e959c
a better title is qml: remove page indicator and navbar from Wizard Control
@@ -19,6 +19,7 @@ Control { | |||
property string subtext | |||
property int subtextMargin | |||
property int subtextSize: 15 | |||
property bool wrap: 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.
you could make this it's own commit:
qml: customizable wrap mode in Header control
diff --git a/src/qml/controls/Header.qml b/src/qml/controls/Header.qml
index 59fb01671..c7adcf309 100644
--- a/src/qml/controls/Header.qml
+++ b/src/qml/controls/Header.qml
@@ -19,6 +19,7 @@ Control {
property string subtext
property int subtextMargin
property int subtextSize: 15
+ property bool wrap: true
contentItem: ColumnLayout {
spacing: 0
Label {
@@ -31,7 +32,7 @@ Control {
color: Theme.color.neutral9
text: root.header
horizontalAlignment: center ? Text.AlignHCenter : Text.AlignLeft
- wrapMode: Text.WordWrap
+ wrapMode: wrap ? Text.WordWrap : Text.NoWrap
}
Loader {
Layout.fillWidth: true
@@ -46,7 +47,7 @@ Control {
color: Theme.color.neutral8
text: root.description
horizontalAlignment: root.center ? Text.AlignHCenter : Text.AlignLeft
- wrapMode: Text.WordWrap
+ wrapMode: wrap ? Text.WordWrap : Text.NoWrap
}
}
Loader {
@@ -62,7 +63,7 @@ Control {
color: Theme.color.neutral9
text: root.subtext
horizontalAlignment: root.center ? Text.AlignHCenter : Text.AlignLeft
- wrapMode: Text.WordWrap
+ wrapMode: wrap ? Text.WordWrap : Text.NoWrap
}
}
}
@@ -9,14 +9,16 @@ Button { | |||
id: root | |||
property int textSize: 18 | |||
property string textColor: Theme.color.neutral9 | |||
property bool bold: 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.
you could make this it's own commit, also maybe switch the logic from defining rightalign
to center
?:
qml: make text boldness and alignment customizable in TextButton control
diff --git a/src/qml/controls/TextButton.qml b/src/qml/controls/TextButton.qml
index 2dce44670..4f508970d 100644
--- a/src/qml/controls/TextButton.qml
+++ b/src/qml/controls/TextButton.qml
@@ -10,13 +10,15 @@ Button {
property int textSize: 18
property string textColor: Theme.color.neutral9
font.family: "Inter"
- font.styleName: "Semi Bold"
+ font.styleName: bold ? "Semi Bold" : "Regular"
font.pixelSize: root.textSize
+ property bool bold: true
+ property bool center: true
contentItem: Text {
text: root.text
font: root.font
color: root.textColor
- horizontalAlignment: Text.AlignHCenter
+ horizontalAlignment: center ? Text.AlignHCenter : Text.AlignRight
verticalAlignment: Text.AlignVCenter
}
background: null
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.
maybe switch the logic from defining
rightalign
tocenter
?
I am afraid I did not understand the change you are referring to. Would you please elaborate on this point?
Thanks!
ACK 5118be4 |
ACK 5118be4 Will pick up feedback in follow-ups |
Observing in terminal:
|
Great! Let me know which one you pick first, and I shall get doing the rest. |
I didn't observe this message while building/running the views. It would greatly help if you could give details on precisely when you observed this statement: when starting the GUI or after clicking on some action in the views. |
It seems like a Wayland-specific issue. |
So is this something we can work on fixing, or is it a bug intrinsic to Wayland? |
IDK :) |
@mouxdesign Do you mean column 2/3? The images below row 2 still show many errors |
I tested this again, and my system shows column 2 |
@Rspigler I believe column 2 is the newer and correct version - so should be all good. Is that right, @mouxdesign? |
100% correct as @GBKS mentioned the second column is the newer version. I see in my initial message I mentioned a row, so understand which the confusion might have come from. The third column are Christoph's original designs. |
ee82e9c qml: remove information qml (jarolrod) 40d48e7 qml: allow the settings component to load a dynamic action item. (jarolrod) Pull request description: #124 Introduced information.qml which is a mutation of `Setting.qml` but introduces an additional two loaders and respective properties to handle the scenarios where a setting doesn't use an option switch and needs to instead display a link, display an image, or take input. The issue, despite large amounts of duplicate code is that, following this design pattern, we will be introducing a new mutation of `Settings.qml` everytime we have a scenario when there should be something aside from an option switch. Instead of endless mutations, `Settings.qml` can just be made extensible by simply introducing a loader which gives it an `actionItem`. an `actionItem` is the interact-able element of this setting; that could be an option switch, a url, a text input box, or something completely new. An example of something new, see the current simplification of the `AboutOptions.qml` developer settings button: ```diff - RowLayout { - Header { - Layout.fillWidth: true - center: false - header: qsTr("Developer options") - headerSize: 18 - description: qsTr("Only use these if you have development experience") - descriptionSize: 15 - descriptionMargin: 10 - wrap: false - } - Loader { - Layout.fillWidth: true - Layout.preferredWidth: 0 - Layout.alignment: Qt.AlignRight - Layout.rightMargin: 5 - active: true - visible: active - sourceComponent: TextButton { - text: ">" - bold: false - rightalign: true - onClicked: { - introductions.incrementCurrentIndex() - swipeView.inSubPage = true - } + Setting { + Layout.fillWidth: true + header: qsTr("Developer options") + description: qsTr("Only use these if you have development experience") + actionItem: TextButton { + text: ">" + bold: false + rightalign: true + onClicked: { + introductions.incrementCurrentIndex() + swipeView.inSubPage = true } ``` [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/143) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/143) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/143) [](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/143) ACKs for top commit: hebasto: re-ACK ee82e9c Tree-SHA512: 0fa95884d25b56ed6d6eaf49f554de502b44b3fe4dc1db17967c1b5fe2b6aa76e3188e00ad00173335a973c3b48e5e4947dc170becbe4a42cdd1417be0fd5c02
This PR adds the views for the onboarding wizard.
Aim:
The main aim of this PR is to introduce the onboarding views and the interactions in it.
Details: