Skip to content

Use OnboardingInfo for all onboarding pages #192

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

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Nov 23, 2022

This PR reworks the OnboardingInfo control so that we can use it for all onboarding pages.

The OnboardingInfo control doesn't work for the onboarding pages which represent the advanced settings views because the OnboardingInfo control always shows the continue button, which the advanced settings views do not use. We get around this here by making the buttonText value optional, making it so that the ContinueButton is not visible or enabled when the continueText value is not specified.

Additionally, this now changes the way that we fix the issue shown in #185. Instead of setting a maxwidth or 490 for the content, we give the Setting control an implicitWidth of 450. This is necessary in order to accommodate this change as we will no longer have an independent Header control which stretches out the width.

Desktop

master

a b c d
Screen Shot 2022-11-23 at 1 40 14 AM Screen Shot 2022-11-23 at 1 40 26 AM Screen Shot 2022-11-23 at 1 40 40 AM Screen Shot 2022-11-23 at 1 40 52 AM

pr

a b c d
Screen Shot 2022-11-23 at 2 56 48 AM Screen Shot 2022-11-23 at 2 56 55 AM Screen Shot 2022-11-23 at 2 57 10 AM Screen Shot 2022-11-23 at 2 57 21 AM

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

@jarolrod jarolrod marked this pull request as draft November 23, 2022 23:10
@jarolrod
Copy link
Member Author

Converting to draft until i fix the width issue on android

@jarolrod jarolrod force-pushed the onboardinginfo-settings-views branch from d6d315b to 139640f Compare December 6, 2022 02:44
@jarolrod
Copy link
Member Author

jarolrod commented Dec 6, 2022

updated from d6d315b to 139640f (pr192.01 -> pr192.02, diff)

changes: address layout issues on mobile by encapsulating the detail in a columnlayout. This means we no longer need the commit that set the setting control's implicitwidth (this was wrong anyways). There is probably a better way to address this, but that can be picked up later.

@jarolrod jarolrod marked this pull request as ready for review December 6, 2022 02:45
@hebasto
Copy link
Member

hebasto commented Dec 6, 2022

Suggesting to rebase to fix CI tasks.

This allows for using the OnboardingInfo control with the onboarding
views that represent advanced setting customization.
All onboarding views are now represented with the OnboardingInfo
control.
@jarolrod jarolrod force-pushed the onboardinginfo-settings-views branch from 139640f to b954e9f Compare December 7, 2022 01:12
@jarolrod
Copy link
Member Author

jarolrod commented Dec 7, 2022

updated from 139640f to b954e9f (pr192.02 -> pr192.03, diff)

changes: rebase over master

Copy link
Collaborator

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

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

ACK b954e9f

Changes look clean and no qml warnings when running.

@hebasto hebasto merged commit 7519f7f into bitcoin-core:main Dec 8, 2022
hebasto added a commit that referenced this pull request Dec 8, 2022
183be5c qml: ensure use of index decrement function instead of arithmetic (Jarol Rodriguez)
8a57f61 qml: ensure translation function for strings in use in onboarding views (jarolrod)
ec0b524 qml: deduplicate onboarding code with encapsulation of page definition (jarolrod)

Pull request description:

  based on #192

  This encapsulates the page logic for the onboarding pages into OnboardingInfo, which is then renamed to InformationPage; because now this can be used for reusable `Settings` pages and not just the onboarding views.

  This has the benefit of removing a ton of duplicated code around page declaration, but also sets us up to introduce the ScrollView and have it be effective for all pages that use InformationPage, which in turn are all of the views aside from the node view.

  Bundled in this change is an update to ensure that we are using the qml translator function for all strings that are a part of the onboarding views.

  ## Desktop
  ### Master

  | master-1 | master-2 | master-3 | master-4 | master-5 |
  | -------- | -------- | -------- | -------- | -------- |
  | <img width="708" alt="master-1" src="https://user-images.githubusercontent.com/23396902/206313556-a2f9b720-368b-41c5-bc13-bb695c83c807.png"> | <img width="752" alt="master-2" src="https://user-images.githubusercontent.com/23396902/206313569-9a90d545-a243-484e-975d-87b5192e7637.png"> | <img width="752" alt="master-3" src="https://user-images.githubusercontent.com/23396902/206313586-fa967658-cb38-47cb-8c25-1d1cc79e244b.png"> | <img width="752" alt="master-4" src="https://user-images.githubusercontent.com/23396902/206313603-da9849eb-8f40-4bdd-a231-de7843e8bd6f.png"> | <img width="752" alt="master-5" src="https://user-images.githubusercontent.com/23396902/206313630-7429fc73-4505-4888-9a00-f03ac7b8a66f.png"> |

  | master-6 | master-7 | master-8 | master-9 | master-10 |
  | -------- | -------- | -------- | -------- | --------- |
  | <img width="752" alt="master-6" src="https://user-images.githubusercontent.com/23396902/206313704-99e33b2a-a7b5-498b-9ae1-b4b77aa343a4.png"> | <img width="752" alt="master-7" src="https://user-images.githubusercontent.com/23396902/206313719-2f5efacf-85c1-436a-8782-0fad0c9d7193.png"> | <img width="752" alt="master-8" src="https://user-images.githubusercontent.com/23396902/206313739-1f02173a-0fac-45ba-af37-338c18e765f7.png"> | <img width="752" alt="master-9" src="https://user-images.githubusercontent.com/23396902/206313756-79072fc9-5af1-43be-b6f3-99dcb402f98f.png"> | <img width="752" alt="master-10" src="https://user-images.githubusercontent.com/23396902/206313767-33b8a2ad-eafc-47b3-a0eb-8a578a55597f.png"> |

  ### PR

  | pr-1 | pr-2 | pr-3 | pr-4 | pr-5 |
  | ---- | ---- | ---- | ---- | ---- |
  | <img width="708" alt="pr-1" src="https://user-images.githubusercontent.com/23396902/206313989-bdb396c2-080a-4635-b46d-40b14cd70cc2.png"> | <img width="752" alt="pr-2" src="https://user-images.githubusercontent.com/23396902/206313996-e2f7ccc4-6d6e-4d4a-9afd-10b765db37db.png"> | <img width="752" alt="pr-3" src="https://user-images.githubusercontent.com/23396902/206314014-2ea1d4e0-4993-41f8-88c5-fe237ef33019.png"> | <img width="752" alt="pr-4" src="https://user-images.githubusercontent.com/23396902/206314034-72fc7a11-8430-4067-874c-5a76224560cc.png"> | <img width="752" alt="pr-5" src="https://user-images.githubusercontent.com/23396902/206314053-02cfd0c3-dad9-4801-a223-82e909ea0918.png"> |

  | pr-6 | pr-7 | pr-8 | pr-9 | pr-10 |
  | ---- | ---- | ---- | ---- | ----- |
  | <img width="752" alt="pr-6" src="https://user-images.githubusercontent.com/23396902/206314169-b40cb04d-40e7-4ce0-8066-df2435ba1c76.png"> | <img width="752" alt="pr-7" src="https://user-images.githubusercontent.com/23396902/206314185-98201cb2-cff9-4d7d-9da9-2b55ac64a8eb.png"> | <img width="752" alt="pr-8" src="https://user-images.githubusercontent.com/23396902/206314200-cd378c86-2306-4984-b53a-359463101f8a.png"> | <img width="752" alt="pr-9" src="https://user-images.githubusercontent.com/23396902/206314220-980af335-a335-4a09-b84d-c993e914a9d6.png"> | <img width="752" alt="pr-10" src="https://user-images.githubusercontent.com/23396902/206314239-3aae7e40-4372-4566-b947-e96114c959aa.png"> |

  [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/193)
  [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/193)
  [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/193)
  [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/193)

ACKs for top commit:
  johnny9:
    ACK 183be5c

Tree-SHA512: bb935791134a9dc6e9deb95b7d3640249150ec3744ce3a941f9c41f39946bdf38510dfe6ed5992ab5acf61f3e916aad97171bb19f8487ef1d819e6a4300a91ce
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
This allows for using the OnboardingInfo control with the onboarding
views that represent advanced setting customization.

Github-Pull: bitcoin-core#192
Rebased-From: 325099d
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
All onboarding views are now represented with the OnboardingInfo
control.

Github-Pull: bitcoin-core#192
Rebased-From: b954e9f
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
This allows for using the OnboardingInfo control with the onboarding
views that represent advanced setting customization.

Github-Pull: bitcoin-core#192
Rebased-From: 325099d
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
All onboarding views are now represented with the OnboardingInfo
control.

Github-Pull: bitcoin-core#192
Rebased-From: b954e9f
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
This allows for using the OnboardingInfo control with the onboarding
views that represent advanced setting customization.

Github-Pull: bitcoin-core#192
Rebased-From: 325099d
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
All onboarding views are now represented with the OnboardingInfo
control.

Github-Pull: bitcoin-core#192
Rebased-From: b954e9f
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.

3 participants