Skip to content

[WIP] Implement onboarding workflow#43

Closed
hebasto wants to merge 3 commits into
bitcoin-core:mainfrom
hebasto:211002-onboard
Closed

[WIP] Implement onboarding workflow#43
hebasto wants to merge 3 commits into
bitcoin-core:mainfrom
hebasto:211002-onboard

Conversation

@hebasto
Copy link
Copy Markdown
Member

@hebasto hebasto commented Oct 2, 2021

This PR implements Bosch-0's design:

Screenshot from 2021-10-02 21-42-07

Based on #42, and only the last commit belongs to this PR.

@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 2, 2021

cc @Bosch-0

Comment thread src/qml/pages/onboarding/Onboarding00Welcome.qml Outdated
Comment thread src/qml/pages/onboarding/Onboarding00Welcome.qml Outdated
Comment thread src/qml/pages/onboarding/Onboarding00Welcome.qml Outdated
promag added a commit to promag/gui-qml that referenced this pull request Oct 3, 2021
Copy link
Copy Markdown
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Pushed my suggestions to https://github.com/promag/gui-qml/tree/qml-43. See

Screen.Recording.2021-10-03.at.15.14.27.mov

@jarolrod
Copy link
Copy Markdown
Contributor

jarolrod commented Oct 3, 2021

Pushed my suggestions to https://github.com/promag/gui-qml/tree/qml-43. See

Screen.Recording.2021-10-03.at.15.14.27.mov

@Bosch-0 @GBKS In regards to this video: Shouldn't the windows have a minimum size, and minimum amount of padding?

@promag
Copy link
Copy Markdown
Contributor

promag commented Oct 3, 2021

Pushed my suggestions to https://github.com/promag/gui-qml/tree/qml-43. See
Screen.Recording.2021-10-03.at.15.14.27.mov

@Bosch-0 @GBKS In regards to this video: Shouldn't the windows have a minimum size, and minimum amount of padding?

Definitely, this is just to show that layouts are superior to anchors.

@jarolrod
Copy link
Copy Markdown
Contributor

jarolrod commented Oct 3, 2021

@promag oh right, sorry!

@hebasto hebasto mentioned this pull request Oct 4, 2021
@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 4, 2021

@promag

Pushed my suggestions to https://github.com/promag/gui-qml/tree/qml-43.

wrapMode: Text.WordWrap does not work as expected.

Co-authored-by: bosch <bosch5@pm.me>
@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 4, 2021

Updated 21c9692 -> 7fc49d7 (pr43.01 -> pr43.02):

  • rebased
  • addressed some of @promag's comments

@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 5, 2021

Updated 7fc49d7 -> 6a19c31 (pr43.02 -> pr43.04):

  • implemented @promag's suggestions to use ColumnLayout and Pane

Added commits are supposed to be squashed before merging.

}

Button {
id: startButton
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed.

color: "black"
}

ColumnLayout {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

contentItem: ColumnLayout {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

}

ColumnLayout {
anchors.centerIn: parent
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed. If you want this vertically aligned while ensuring minimum height then do this:

contentItem: ColumnLayout {
    Item {
        Layout.fillHeight: true
    }
    // ... items here
    Item {
        Layout.fillHeight: true
    }
}

Same concept as https://doc.qt.io/qt-5/qspaceritem.html

import QtQuick.Layouts 1.12

Pane {
background: Rectangle {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed? The window background is already black. To improve rendering time make background: null

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To improve rendering time make background: null

Is it documented? (I failed to find out)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Documentation for null or for rendering improvement?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Documentation for null or for rendering improvement?

The former.


Image {
Layout.alignment: Qt.AlignCenter
source: "image://images/app"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

making a note that @Bosch-0 has a proposal for a new icon. It needs to be discussed at greater length with the community; if there is consensus we should use it: bitcoin-core/gui#199

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not just a new icon, there is also some changes to the watermark and thus overall logo. See this file for details: https://www.figma.com/file/c1V7b23n0LqRbVJlUkE1mn/Bitcoin-Core-App-Bosch?node-id=1850%3A44

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new proposed icon is visually not centered in the circle. Can we address that, please?

Also, there are two oranges in the Figma file, Bitcoin Core orange and Bitcoin orange. They are slightly different, which one is the "right one"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I updated that awhile ago, looks visually centered to me?

image

The Bitcoin Core orange is the primary orange, the Bitcoin orange is just there to illustrate the orange used by the Bitcoin logo - ill remove it from the stylesheet.


Label {
Layout.maximumWidth: appTitle.width
text: "Be part of the Bitcoin network."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wrap in qsTr in preparation for translation support?

Suggested change
text: "Be part of the Bitcoin network."
text: qsTr("Be part of the Bitcoin network.")

@Rspigler
Copy link
Copy Markdown

Really exciting work! Concept ACK

@hebasto hebasto marked this pull request as draft January 6, 2022 13:47
@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Jun 3, 2022

Closing in favor of #124.

@hebasto hebasto closed this Jun 3, 2022
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.

6 participants