Details
- Reviewers
ngraham mbruchert - Group Reviewers
Breeze - Commits
- R266:7461f90af829: Add view-barcode-qr icons
Diff Detail
- Repository
- R266 Breeze Icons
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14297 Build 14315: arc lint + arc unit
Hi! Thanks for the patch. There are few things I'd like you to change before I accept this.
The icons need optimization and colorscheme support (see workflow tips). The colors are still hardcoded because inkscape does that. If you have questions or need help, feel free to ask.
16px version:
The margins are a pixel too wide on each side (see HIG). You could even fit in some random dots with the extra space that decreasing the margins would give you.
32px:
Not sure if this should be changed, but what is the reason for the blue corners?
Once everything else is done, make sure you copy the icons into the icons-dark/ folder and change the #232629 color to #eff0f1
Make sure you read these:
https://hig.kde.org/style/icon.html
https://community.kde.org/Guidelines_and_HOWTOs/Icon_Workflow_Tips
How about view-barcode-qr? then we could potentially have specific view-barcode-aztec and also a fallback to view-barcode
But aren't QR codes normally scannable?
Nice job on the icons BTW. I like the way the 16px version looks now.
I think I do want the 32px version to not have the blue corners so that it is visually consistent with the smaller versions. Once that is done, I will accept this, as long as there aren't any issues with the colorscheme support.
I'm currently on vacation and can't access my laptop. Could you show me a screenshot of the 32px icon at 1x size?
I noticed that only the 22px icons use the viewbox, is this intentional?
As for the 32px one the validator I used complains that:
Error: Attribute paint-order not allowed on SVG element path at this point. From line 7, column 5; to line 7, column 1521 tyle>↩ <path class="ColorScheme-Text" fill="currentColor" style="fill:currentColor;fill-opacity:1;stroke:no… 16v-2h16v2zm16 0h-2V3h2zm0-16v2h-16V3z" paint-order="markers fill stroke" transform="rotate(-90)"/>↩</s
viewbox vs height/width doesn't matter, the result is the same. It looks like the extra fill currentColor needs to be removed.
Functionally it looks good to me, there are a few improvements/optimizations that can be made for example 16 and 22 have unnecessary attributes such as:
fill="currentColor" style="fill:currentColor;
And the 32 one has a transform that can be applied:
<svg xmlns="http://www.w3.org/2000/svg" id="svg938" width="54" height="54"><style id="current-color-scheme">.ColorScheme-Text{color:#232629}</style><path fill="currentColor" stroke-width="2" d="M27 9V7h2v2zm0 6v-2h2v2zm-4 2v-4h2v4zm24 24v2h-2v-2zm2-2v2h-2v-2zm-8 6v2h-4v-2zm-2 0v2h-2v-2zm0 4v2h-2v-2zm4-2v4h-2v-4zm8 4h-4v-2h4zm-4-4v4h-2v-4zm2-2v2h-6v-2zm2-4v4h-2v-4zm-18 2h2v2h-2zm2 2h2v2h-2zm-4 4v-2h6v2zm12-14h-2v-2h2zm0-2h-4v-2h4zm-6 0h-2v-2h2zm-6 2h2v4h-2zm0 4h2v2h-2zm20-12h-2v-4h2zm0-2h-2v-2h2zm-2-2h-6v-2h6zm-2 2h-2v-2h2zm2 4h-4v-2h4zm-4 8v-4h2v4zm4-4h-4v-2h4zm2 2h-2v-6h2zm0 4h-4v-2h4zm-16-8h-2v-4h2zm4-8h-2v-2h2zm-2 4h-4v-4h4zm2 4h-2v-2h2zm0-2v-4h2v4zm4-4h-4v-2h4zm2 2h-2v-2h2zm0 4h-4v-2h4zM13 21h2v4h-2zm10 8h2v2h-2zm-2-2h2v2h-2zm0 22h6v2h-6zm10 0h2v2h-2zm-10-4h2v2h-2zm2 2h2v2h-2zm4-4v4h-2v-4zm0 4h2v2h-2zm2 0v-8h2v8zm-8-8h8v2h-8zm14 2h6v2h-6zm6 2v-8h2v8zm0-6h-6v-2h6zm-4-2v6h-2v-6zm-10-2v4h-2v-4zm-4 4h4v2h-4zm-8-8h4v2h-4zm6 2h4v2h-4zm10-2v-2h2v2zm-4 4v-4h2v4zm2 2v-8h2v8zM25 5V3h2v2zm0 8v-2h2v2zM9 29h2v2H9zm14-8h4v2h-4zm-6 0h4v2h-4zm2 2h4v2h-4zm-4 2h14v2H15zm-4 0h4v4h-4zm-2-4h2v2H9zm0 2v4H7v-4zm-4 4h4v2H5zm-2-2h2v2H3zm0 6h8v2H3zm0-10h4v2H3zm24-2v-2h2v2zm6 0H21v2h12zm-12 0v-8h2v8zm8-2v-2h2v2zm2 4V11h2v10zm-2-10V9h2v2zm2-2V7h2v2zm-2-2V5h2v2zm0-2V3h4v2zm-8 4V5h2v4zm2 2V3h2v8zM7 47v-8h8v8zm-4 4v-2h16v2zm16 0h-2V35h2zm0-16v2H3v-2zM3 35h2v16H3zm36-20V7h8v8zm-4 4v-2h16v2zm16 0h-2V3h2zm0-16v2H35V3zM35 3h2v16h-2zM7 15V7h8v8zm-4 4v-2h16v2zm16 0h-2V3h2zm0-16v2H3V3zM3 3h2v16H3z" class="ColorScheme-Text"/></svg>
I used svgo to make these optimizations.
I try not to be too aggressive with optimization when it comes to diffs from other people because the process of making an icon is currently quite tedious and the optimizations can be pretty small after the metadata is removed.
icons-dark/actions/16/view-barcode-qr.svg | ||
---|---|---|
7 ↗ | (On Diff #62915) | delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2" |
icons-dark/actions/22/view-barcode-qr.svg | ||
7 ↗ | (On Diff #62915) | delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2.23606801" |
icons-dark/actions/32/view-barcode-qr.svg | ||
7 ↗ | (On Diff #62915) | delete style="fill-opacity:1;stroke:none;stroke-width:2" |
icons/actions/16/view-barcode-qr.svg | ||
7 ↗ | (On Diff #62915) | delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2" |
icons/actions/22/view-barcode-qr.svg | ||
7 ↗ | (On Diff #62915) | delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2.23606801" |
icons/actions/32/view-barcode-qr.svg | ||
7 ↗ | (On Diff #62915) | delete style="fill-opacity:1;stroke:none;stroke-width:2" |
actually, you can disregard my inline comments on the 32px icons, I didn't realize you had already removed the duplicate fill currentColor bits. BTW, none of the code for style= matters as long as you have fill="currentColor" and the color class since we're not using strokes or opacity.
Agreed, and it should be automated. I posted the optimizations above because they could serve as a base to make the other ones and it doesn't reduce readability (it only applies the -90° rotation) :)
The 32px icons need to be 32x32, not 54x54.
Also see this to make sure you've got the margins right.
https://hig.kde.org/_downloads/817f2d10252d9449f149253e49e3796b/icon_margins-monochrome-8x.png