Skip to content

Use caret-right icon instead of greater-than text symbol#144

Merged
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:use-right-caret
Aug 13, 2022
Merged

Use caret-right icon instead of greater-than text symbol#144
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:use-right-caret

Conversation

@jarolrod
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod commented Aug 3, 2022

Mainly only relevant in the AboutOptions component for now.

master pr
Screen Shot 2022-08-03 at 6 45 51 AM Screen Shot 2022-08-06 at 7 40 39 PM

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

Copy link
Copy Markdown
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.

Concept ACK

  • I can see the visual difference between master and PR while testing.

Screenshot:

Master PR
Screenshot from 2022-08-03 21-04-59 Screenshot from 2022-08-03 21-05-27
  • This PR also solves the issue of the misaligned arrow of the developer options selector.

Suggestion:

  • There is one more instance of setting which need to be addressed here. The ">" for the Proxy settings option on the Connection Settings page should be converted to the icon.

@jarolrod jarolrod marked this pull request as draft August 4, 2022 03:06
@Rspigler
Copy link
Copy Markdown

Rspigler commented Aug 4, 2022

tACK commit 1c17b77

I can see the change and it is all lined up on my machine. The one missing the change is the Proxy setting under 'Connection Settings' as @shaavan mentions

@jarolrod jarolrod marked this pull request as ready for review August 6, 2022 23:42
@jarolrod
Copy link
Copy Markdown
Contributor Author

jarolrod commented Aug 6, 2022

updated from from 1c17b77 to 1dcdac6 (pr144.01 -> pr144.02, diff)

Changes:

  • rebased over changes in PR's this PR is based on
  • Updated icon sources to use image:// syntax instead of qrc syntax, as the image:// syntax is required for the image provider to work and provide a proper resolution image. Comparison below (top is image:// and below that is qrc:

Screen Shot 2022-08-06 at 6 58 51 PM

@jarolrod
Copy link
Copy Markdown
Contributor Author

updated from 1dcdac6 to 36bc09d Compare

Changes: give the developer options setting button the correct action, where it will actually open the developer options subpage

@jarolrod
Copy link
Copy Markdown
Contributor Author

updated from 36bc09d to f9f6921

changes: updated over changes on master

Comment on lines +61 to +65
icon.source: "image://images/caret-right"
icon.color: Theme.color.neutral9
icon.height: 18
icon.width: 18
background: null
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are the icon.color and background properties are specified for some elements, but not for others?

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.

Button is a qml type, so we need to specify the icon.color, background value explicitly for each time the Button is used as actionItem, which this PR does.

Whereas, the ExternalLink is a custom-defined component that already employs the respective icon.color, and background value in its definition, so we need not specify that separately when using it.

In src/qml/controls/ExternalLink.qml

            sourceComponent: Button {
                icon.source: root.iconSource
                icon.color: Theme.color.neutral9
                icon.height: root.iconHeight
                icon.width: root.iconWidth
                background: null
                onClicked: Qt.openUrlExternally(link)
            }

Copy link
Copy Markdown
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.

ACK f9f6921

The ExternalLink and Button components replace all the instances of greater-than text symbol and are used appropriately.

I have two ideas to discuss that we can consider incorporating in this PR or in a follow-up.

  1. The Button{} is used twice as an actionItem in the current implementation of GUI. There is a chance that we could need to use this Button configuration multiple times as the development progresses. So we can think about extracting this code as a separate component.
  2. There is only one instance of usage of ExternalLink{} where the default values for iconWidth, and iconHeight (30, 30) are used. All other usage instances modify these values to (18, 18). Changing the default values in ExternalLink.qml for these properties will save up on redundant code.

@hebasto hebasto merged commit bae89e3 into bitcoin-core:main Aug 13, 2022
@jarolrod jarolrod deleted the use-right-caret branch August 21, 2022 19:44
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
johnny9 pushed a commit to johnny9/bitcoin-core-app that referenced this pull request Jul 4, 2025
…er-than text symbol

e17241d qml: use caret-right icon instead of greater than symbol (jarolrod)

Pull request description:

  Mainly only relevant in the `AboutOptions` component for now.

  | master | pr |
  | ------- | -- |
  | <img width="752" alt="Screen Shot 2022-08-03 at 6 45 51 AM" src="https://user-images.githubusercontent.com/23396902/182590655-4e765f99-8de4-4782-bc12-6682b63b1eeb.png"> | <img width="752" alt="Screen Shot 2022-08-06 at 7 40 39 PM" src="https://user-images.githubusercontent.com/23396902/183269335-b8dd6894-14e9-47b5-8d43-6d2f540cbb27.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/144)
  [![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/144)
  [![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/144)
  [![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/144)

ACKs for top commit:
  shaavan:
    ACK e17241d

Tree-SHA512: ce2a5fd72a1e8f84d1d0d5ddf8fb06f49e2bce684dd3db603a4c9aa937f45d2e13bbc1f279c70178f3cc117cd9cd71db522e1b7bd20f7b41aea5db8ec7bb5b69
tx-signer450 added a commit to tx-signer450/gui-qml that referenced this pull request Oct 20, 2025
…er-than text symbol

e17241df0c355d808226abfd608b7408646eb20b qml: use caret-right icon instead of greater than symbol (jarolrod)

Pull request description:

  Mainly only relevant in the `AboutOptions` component for now.

  | master | pr |
  | ------- | -- |
  | <img width="752" alt="Screen Shot 2022-08-03 at 6 45 51 AM" src="https://user-images.githubusercontent.com/23396902/182590655-4e765f99-8de4-4782-bc12-6682b63b1eeb.png"> | <img width="752" alt="Screen Shot 2022-08-06 at 7 40 39 PM" src="https://user-images.githubusercontent.com/23396902/183269335-b8dd6894-14e9-47b5-8d43-6d2f540cbb27.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/144)
  [![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/144)
  [![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/144)
  [![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/144)

ACKs for top commit:
  shaavan:
    ACK e17241df0c355d808226abfd608b7408646eb20b

Tree-SHA512: ce2a5fd72a1e8f84d1d0d5ddf8fb06f49e2bce684dd3db603a4c9aa937f45d2e13bbc1f279c70178f3cc117cd9cd71db522e1b7bd20f7b41aea5db8ec7bb5b69
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.

4 participants