Skip to content

doc: introduce icon policy documentation#70

Merged
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:icon-policy-doc
Nov 2, 2021
Merged

doc: introduce icon policy documentation#70
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:icon-policy-doc

Conversation

@jarolrod
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod commented Oct 26, 2021

Moved from bitcoin-core/gui#310

Picking up bitcoin-core/gui#178

This introduces documented policies for how icons should be prepared, optimized, styled, contributed, and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient.

document render

@hebasto hebasto added the Doc Improvements or additions to documentation label Oct 26, 2021
@jarolrod
Copy link
Copy Markdown
Contributor Author

cc @Bosch-0 @GBKS

Copy link
Copy Markdown
Contributor

@Bosch-0 Bosch-0 left a comment

Choose a reason for hiding this comment

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

Could we not have that PNG optimization tool run automatically when an image / icon is added to the repo?

@promag
Copy link
Copy Markdown
Contributor

promag commented Oct 27, 2021

@Bosch-0 where would it run automatically? We don't want non-optimized icons in the source tree so the command would have to run client-side. I think you could configure a pre-commit hook in your git client to do this automatically but maybe not worth the effort.

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.

Concept ACK.


#### SVG Source File
SVGs are used as source files because they can scale while retaining image quality.
They are not used in production due to limited application support.
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.

Do you have an example of limited app support?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line was originally written by @Bosch-0

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 can think of 2 motivations:

  1. From https://doc.qt.io/qt-5/svgrendering.html:

Qt supports the static features of SVG 1.2 Tiny. ECMA scripts and DOM manipulation are currently not supported.

  1. rendering an SVG in a small size might not produce a good-looking icon and so it is easier to have a PNG optimized for that size.

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.

Limited app support may be too harsh, not as widely supported as PNG is probably a better way to put it. JPEG files should also be used over SVGs for images with wider color profiles.

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 phrase “limited application support.” sounds a little confusing and might be misleading. For example, one might think, which application are we talking about here? Is it “bitcoin core” or some other application that works on PNG, SVG, and other formats of images.

I think a better framing of this sentence would be:
“They are not used in production because they are not widely supported (compared to PNGs).”

In this framing, the reader will clearly understand that we are talking about format support in general.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line is just a quick explanation as to why we are not using SVG files as our production files. The reason for this is because of limited application support; this could refer to x, y, or z reason.

In reality, we are/will be using SVG as production files because that is what designers have recommended. Getting into the particularities here, or what the reality of support for SVG files really is across all applications doesn't matter.

@shaavan: Changing from "limited application support" to "not widely supported" doesn't remove the inherent reference to 'outside' projects

I think the line is fine as is

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.

Changing from "limited application support" to "not widely supported" doesn't remove the inherent reference to 'outside' projects

I can see what you are talking about. I guess the phrase "limited application support" is not as confusing as I first perceived it to be :)

Comment thread src/qml/doc/icon-policy.md Outdated
@jarolrod
Copy link
Copy Markdown
Contributor Author

Updated from 8a2c366 -> 23ad199

Changes: addressed @promag comment

@hebasto hebasto added the UX Designers' opinions are required label Oct 30, 2021
Copy link
Copy Markdown
Member

@hebasto hebasto 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.

and text chunks, resulting in a lossless reduction of the file's size.

## Contributing
Bitcoin Core primarily uses icons from the [Bitcoin Icon set](https://github.com/BitcoinDesign/Bitcoin-Icons),
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.

In the current state of this repo no icons are from the Bitcoin Icon set, right? Maybe clarify that new added icons should be chosen from that set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We will be using those Icons. Although there are none yet, they will be added as needed. We know that we will primarily use these icons. I don't see the need to state, as of right now, that "new" icons will be chosen from the set.

Comment thread src/qml/doc/icon-policy.md Outdated
Comment thread src/qml/doc/icon-policy.md Outdated
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

The icon policy documentation is quite nicely arranged. The details in the document are pretty well explained without being overwhelming. This writing style fulfills the purpose of the documentation as it would be used chiefly by newcomers in the bitcoin core contributing ecosystem.

I have one change to suggest, which I have mentioned here. Other than this and other minor changes suggested by other reviewers, I think this documentation is perfect!

@jarolrod
Copy link
Copy Markdown
Contributor Author

jarolrod commented Nov 1, 2021

Updated from 23ad199 -> 68efd37 (pr70.02 -> pr70.03, diff)

Changes:

@Bosch-0
Copy link
Copy Markdown
Contributor

Bosch-0 commented Nov 2, 2021

ACK from me :)

Copy link
Copy Markdown
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 68efd37

Comment thread src/qml/doc/icon-policy.md Outdated
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 68efd37

Thanks again for this fantastic work, @jarolrod!

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.

ACK 68efd37.

+1 to @hebasto comment though.

Co-authored-by: jarolrod <jarolrod@tutanota.com>
@jarolrod
Copy link
Copy Markdown
Contributor Author

jarolrod commented Nov 2, 2021

updated from 68efd37 -> 7335bdd (pr70.03 -> pr70.04 ,diff)

Changes: addressed @hebasto comment

Copy link
Copy Markdown
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 7335bdd

@hebasto hebasto merged commit 000ff6b into bitcoin-core:main Nov 2, 2021
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 6, 2025
Co-authored-by: jarolrod <jarolrod@tutanota.com>

Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 7, 2025
Co-authored-by: jarolrod <jarolrod@tutanota.com>

Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 7, 2025
Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd

Co-authored-by: jarolrod <jarolrod@tutanota.com>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd

Co-authored-by: jarolrod <jarolrod@tutanota.com>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd

Co-authored-by: jarolrod <jarolrod@tutanota.com>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd

Co-authored-by: jarolrod <jarolrod@tutanota.com>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Github-Pull: bitcoin-core#70
Rebased-From: 7335bdd

Co-authored-by: jarolrod <jarolrod@tutanota.com>
johnny9 pushed a commit to johnny9/bitcoin-core-app that referenced this pull request Jul 4, 2025
40e6a0e doc: introduce icon policy documentation (bosch)

Pull request description:

  Moved from bitcoin-core/gui#310

  Picking up bitcoin-core/gui#178

  This introduces documented policies for how icons should be prepared, optimized, styled, contributed, and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient.

  [document render](https://github.com/bitcoin-core/gui-qml/blob/23ad199b43e9068d0571eba5965752b03842a7f4/src/qml/doc/icon-policy.md)

ACKs for top commit:
  hebasto:
    re-ACK 40e6a0e

Tree-SHA512: aebdfcfea83d23584ccbcf7bdb706bcddfd63dbdb53018ec8220cab87cc92720fd41cb532325e258fff6bcc53b5cebbe498fba2ad95ebebb5da4f868f92607d3
tx-signer450 added a commit to tx-signer450/gui-qml that referenced this pull request Oct 20, 2025
40e6a0e8a279323cf941ff0bde38fd34fcc0f445 doc: introduce icon policy documentation (bosch)

Pull request description:

  Moved from bitcoin-core/gui#310

  Picking up bitcoin-core/gui#178

  This introduces documented policies for how icons should be prepared, optimized, styled, contributed, and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient.

  [document render](https://github.com/bitcoin-core/gui-qml/blob/23ad199b43e9068d0571eba5965752b03842a7f4/src/qml/doc/icon-policy.md)

ACKs for top commit:
  hebasto:
    re-ACK 40e6a0e8a279323cf941ff0bde38fd34fcc0f445

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

Labels

Doc Improvements or additions to documentation UX Designers' opinions are required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants