Skip to content

Make External Link settings icon follow the design file specs #239

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
Feb 13, 2023

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Feb 1, 2023

This replaces the current export icon we have (export-filled) for the export-outline icon, as that is what the design file now specifies. The new icon has been passed through the optimize-png.py script, which output the following:

optimizing export.png... done
summary:
+++++++++++++++++
export.png
  size diff from: 1371 to: 785
  old sha256: c93640af06071bcee055727bcfd01f8576d657260a385035dac8f2f1568cf571
  new sha256: c0605b933dfaae15b26254d08d11d2e9e9e4b20d5a9748386cd9a9f680502266

completed. Checksum stable: False. Total reduction: 586 bytes

Additionally, this adjusts the spacing between the external link text and the icon in order to better match the design file

master

light dark
Screen Shot 2023-01-31 at 7 08 22 PM Screen Shot 2023-01-31 at 7 08 28 PM

pr

light dark
Screen Shot 2023-02-09 at 1 42 16 PM Screen Shot 2023-02-09 at 1 42 22 PM

layout

external icon right caret
Screen Shot 2023-02-09 at 1 41 33 PM Screen Shot 2023-02-09 at 1 41 44 PM

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

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.

The new icon expanded the spacing on the layout. I think adjusting spacing from 20 to 15 in AboutOptions and Settings.qml fixes it but unsure of the side effects of that with the other views

@jarolrod
Copy link
Member Author

jarolrod commented Feb 1, 2023

@johnny9 @GBKS

Is that really an issue? There's no clear spec for how tall a setting should be, for example settings with descriptions are taller

@GBKS
Copy link
Contributor

GBKS commented Feb 2, 2023

List items can indeed scale vertically to fit whatever content needs to be shown. I tried to shoot for a default height of 50px for items that have just one line of text and icon left/right. 44 is the minimum hit area recommended for mobile, and 50 is just a little more convenient. Too much space, and you end up with pretty tall screens with somewhat low information density.

@hebasto
Copy link
Member

hebasto commented Feb 4, 2023

Rebase?

@jarolrod jarolrod mentioned this pull request Feb 5, 2023
@jarolrod jarolrod marked this pull request as draft February 9, 2023 09:02
Currently we included the export filled icon from the icon set, but the
design file now specifies that we use the export-outline icon as the
export icon.
@jarolrod jarolrod marked this pull request as ready for review February 9, 2023 20:44
@jarolrod
Copy link
Member Author

jarolrod commented Feb 9, 2023

Updated from 3874d63 to 515e2fa, compare

Changes:

  • rebased over main to resolve conflicts
  • Fixed layout issue by getting us as close to the design file as I can without spending too many more hours over 2 pixels :D

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 515e2fa

@hebasto hebasto merged commit 876548d into bitcoin-core:main Feb 13, 2023
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