Add view-barcode-qr icons
ClosedPublic

Authored by ndavis on Jul 22 2019, 4:32 PM.

Details

Diff Detail

Repository
R266 Breeze Icons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mbruchert created this revision.Jul 22 2019, 4:32 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2019, 4:32 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mbruchert requested review of this revision.Jul 22 2019, 4:32 PM
ndavis added a subscriber: ndavis.EditedJul 22 2019, 4:55 PM

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

ndavis requested changes to this revision.Jul 22 2019, 4:59 PM
This revision now requires changes to proceed.Jul 22 2019, 4:59 PM
mbruchert updated this revision to Diff 62343.Jul 22 2019, 7:02 PM

Optimize and use css theme colors

mbruchert updated this revision to Diff 62344.Jul 22 2019, 7:04 PM

Remove accidentially committed changes

mbruchert updated this revision to Diff 62346.Jul 22 2019, 7:09 PM

Add icons to breeze-dark

The blue corners are supposed to indicate that the QR-Code can be scanned.

How about view-barcode-qr? then we could potentially have specific view-barcode-aztec and also a fallback to view-barcode

How about view-barcode-qr? then we could potentially have specific view-barcode-aztec and also a fallback to view-barcode

+1

ndavis added a comment.EditedJul 23 2019, 3:22 AM

The blue corners are supposed to indicate that the QR-Code can be scanned.

But aren't QR codes normally scannable?

Nice job on the icons BTW. I like the way the 16px version looks now.

mbruchert updated this revision to Diff 62460.EditedJul 24 2019, 9:08 AM

rename to view-barcode-qr as suggested

mbruchert retitled this revision from Add view-qrcode icons to Add view-barcode-qr icons.Jul 24 2019, 2:37 PM

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

+1

mbruchert updated this revision to Diff 62754.Jul 29 2019, 6:21 PM
  • remove blue corners
mbruchert updated this revision to Diff 62759.Jul 29 2019, 6:39 PM

remove corners

mbruchert updated this revision to Diff 62761.Jul 29 2019, 6:55 PM

remove corners

ndavis added a comment.EditedJul 30 2019, 12:13 AM

I'm currently on vacation and can't access my laptop. Could you show me a screenshot of the 32px icon at 1x size?

lavender added a subscriber: lavender.EditedJul 30 2019, 11:03 AM

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
ndavis added a comment.EditedJul 30 2019, 11:35 AM

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.

mbruchert updated this revision to Diff 62913.Aug 1 2019, 5:20 PM
  • remove corners
mbruchert updated this revision to Diff 62915.Aug 1 2019, 5:25 PM

remove paint-order

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.

[snip]
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.

ngraham accepted this revision.Aug 2 2019, 12:47 PM

LGTM! Let's wait for @ndavis's full review before landing thiw

ndavis added inline comments.Aug 2 2019, 12:52 PM
icons-dark/actions/16/view-barcode-qr.svg
8

delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2"

icons-dark/actions/22/view-barcode-qr.svg
8

delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2.23606801"

icons-dark/actions/32/view-barcode-qr.svg
8

delete style="fill-opacity:1;stroke:none;stroke-width:2"

icons/actions/16/view-barcode-qr.svg
8

delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2"

icons/actions/22/view-barcode-qr.svg
8

delete style="fill:currentColor;fill-opacity:1;stroke:none;stroke-width:2.23606801"

icons/actions/32/view-barcode-qr.svg
8

delete style="fill-opacity:1;stroke:none;stroke-width:2"

ndavis added a comment.EditedAug 2 2019, 12:55 PM

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.

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.

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) :)

ndavis added a comment.EditedAug 2 2019, 6:59 PM

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

ndavis commandeered this revision.Sun, Aug 25, 1:30 PM
ndavis edited reviewers, added: mbruchert; removed: ndavis.
This revision is now accepted and ready to land.Sun, Aug 25, 1:30 PM
ndavis updated this revision to Diff 64574.Sun, Aug 25, 1:31 PM
  • Make 32px be 32px
ndavis marked 6 inline comments as done.Sun, Aug 25, 1:32 PM
This revision was automatically updated to reflect the committed changes.