Skip to content

Condense onboarding pages #149

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Condense onboarding pages #149

wants to merge 1 commit into from

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Aug 10, 2022 •
edited
Loading

This condenses the subpages back into the main file for an onboarding view. We can do this now that elements of a view page are in proper components.

The biggest onboarding page, including all subpages, is now only 119 lines.

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

@jarolrod jarolrod mentioned this pull request Aug 11, 2022
Merged
@jarolrod jarolrod force-pushed the condense-onboarding-pages branch from 177e47e to 1dd2b4d Compare August 12, 2022 04:45
@jarolrod
Copy link
Member Author

updated from 177e47e to 1dd2b4d

changes: rebased over master

hebasto added a commit that referenced this pull request Aug 12, 2022
@hebasto
Merge #150: Temporary dark mode button
9ba04a8 
c231381 qml: temporary dark mode button (jarolrod)

Pull request description:

  A temporary placement for a button that toggles dark mode on and off within the entire UI. Necessary to visually ensure the UI is looking the way that the designers intend. For example, this rebased on top of #149:

  ![current-ui-darkmode](https://user-images.githubusercontent.com/23396902/184046910-953acfdc-c72c-4f34-956a-a7575e2f7c9e.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/150)
  [![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/150)
  [![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/150)
  [![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/150)

ACKs for top commit:
  hebasto:
    ACK c231381, tested on Ubuntu 22.04.

Tree-SHA512: a8d761801961b8f0e60a09c41504a77c328ea25a0c64f937a9f8609b2741a1cba8428ce0c092ffc704e19b7ec385a4d380e99c719ef62c466fb63b85ae5e4831
@jarolrod jarolrod force-pushed the condense-onboarding-pages branch from 1dd2b4d to 454654c Compare August 13, 2022 20:23
@jarolrod
Copy link
Member Author

updated from 1dd2b4d to 454654c

changes: rebased over master and changes on dependent PRs

@jarolrod jarolrod force-pushed the condense-onboarding-pages branch from 454654c to 846a955 Compare August 14, 2022 18:43
@jarolrod
Copy link
Member Author

jarolrod commented Aug 14, 2022 •
edited
Loading

updated from 454654c to 846a955

Changes: rebased over changes on master

This is an opinionated take on how to keep the onboarding view pages tidy and compact, and is not a required change.

shaavan
shaavan reviewed Aug 15, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

I prefer the implementation on the main over this PRs implementation for the following reason.

  1. The swipeView in the current implementation allows each subpage to have there own file. And considering the folder structure of src/qml/pages, I think it is better to keep the pages’ implementation separate.
  2. The subpages are like branched routes from the main onboarding route. Naming these files like “onboarding1a, onboarding1b” emphasizes this point more strongly than keeping them in the same file.

In the end, this is my first take on this PR, and I would like to listen to your comments on the above points I presented.

@jarolrod
qml: condense onboarding view pages
e2750c3 
Condenses subpages into the main page for an onboarding view.
@jarolrod jarolrod force-pushed the condense-onboarding-pages branch from 846a955 to e2750c3 Compare September 26, 2022 18:07
@jarolrod
Copy link
Member Author

updated from from 846a955 to e2750c3

changes: rebased over master

@GBKS
Copy link
Contributor

GBKS commented Oct 26, 2022

An only somewhat related comment, file names are easier to work with when they are descriptive like "onboarding-01-developer-options" rather than just "123" and "abc" style ("onboarding01a", "onboarding01b"). Feel free to ignore as I am not sure what agreed-upon standards are.

@shaavan
Copy link
Contributor

shaavan commented Nov 11, 2022

I think this PR needs quick back-and-forth discussions from the team. So I suggest taking this PR in the next Bitcoin core App Design call.

@jarolrod
Copy link
Member Author

Better to condense and rename into descriptive names, then just condense. Closing for now as not a priority.

@jarolrod jarolrod closed this Nov 11, 2022
@jarolrod jarolrod mentioned this pull request Dec 7, 2022
Merged
hebasto added a commit that referenced this pull request Dec 14, 2022
@hebasto
Merge #198: Do not dynamically load OnboardingViews from url, which n…
18dfb2c 
…ow have descriptive names

6c29ddf qml: remove wizard control (Jarol Rodriguez)
8619e50 qml: do not load onboarding pages from url (Jarol Rodriguez)
5120735 qml: remove unused inSubPage bool (Jarol Rodriguez)
9cc991f qml: give onboarding views descriptive names (Jarol Rodriguez)

Pull request description:

  This is addressing the [desire for descriptive names](#149 (comment)) for the onboarding views as well as (partially) addressing an old piece of feedback from **promag** [about loading pages](#106 (comment)).

  In regards to the second issue this is addressing, the current state of the PR doesn't perfectly address this as there doesn't seem to be a way of given a SwipeView children after it's already been declared. So this PR just doesn't use the Wizard control anymore (which already has limited value and is vulnerable to removal), and deletes it as well.

  ## 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/206323540-7ad151f7-574c-4ed5-ae1e-8668ded70c50.png"> | <img width="752" alt="pr-2" src="https://user-images.githubusercontent.com/23396902/206323554-6d39bdda-5b30-4653-914c-59b3d03d2e2e.png"> | <img width="752" alt="pr-3" src="https://user-images.githubusercontent.com/23396902/206323568-b80db1f1-8ee0-4df2-b86a-b45fa1300c86.png"> | <img width="752" alt="pr-4" src="https://user-images.githubusercontent.com/23396902/206323585-175a8a5e-814a-4260-8ce3-63af9da4dbbb.png"> | <img width="752" alt="pr-5" src="https://user-images.githubusercontent.com/23396902/206323601-43606a1f-83c5-44d6-b891-231951bba47b.png"> |

  | pr-6 | pr-7 | pr-8 | pr-9 | pr-10 |
  | ---- | ---- | ---- | ---- | ----- |
  | <img width="752" alt="pr-6" src="https://user-images.githubusercontent.com/23396902/206323654-d96852e3-0879-40aa-92a3-8a324107d1b5.png"> | <img width="752" alt="pr-7" src="https://user-images.githubusercontent.com/23396902/206323666-4aaa68d9-2e60-45d6-98ae-e580125c55b3.png"> | <img width="752" alt="pr-8" src="https://user-images.githubusercontent.com/23396902/206323683-26bbfdca-a692-41cd-869e-1b95654ff80f.png"> | <img width="752" alt="pr-9" src="https://user-images.githubusercontent.com/23396902/206323695-c2b4bc04-1d4a-4096-9081-1392b923458e.png"> | <img width="752" alt="pr-10" src="https://user-images.githubusercontent.com/23396902/206323710-8ace360d-0d62-4597-9963-6e1df3ca6216.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/198)
  [![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/198)
  [![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/198)
  [![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/198)

ACKs for top commit:
  johnny9:
    ACK 6c29ddf

Tree-SHA512: bacd3419deb0c68652eb0c470fd72f61e424c6872ff17f3bf7adf2848191b42e69627e79b75734b11fa917c3e2fd79288b764bfd4cf318f4711ebaea317b8734
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaavan shaavan shaavan left review comments

Assignees
No one assigned
Labels
Needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jarolrod @GBKS @shaavan @hebasto