Add 48px places icons
ClosedPublic

Authored by manueljlin on May 14 2020, 9:44 AM.

Details

Summary

Everything added except for both trash icons because
I couldn't figure out how to use svg cleaner without
getting rid of the gradient.

BUG:421144
FIXED-IN: 5.71

Diff Detail

Repository
R266 Breeze Icons
Branch
48px-places (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26931
Build 26949: arc lint + arc unit
manueljlin created this revision.May 14 2020, 9:44 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 14 2020, 9:44 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
manueljlin requested review of this revision.May 14 2020, 9:44 AM
manueljlin edited the summary of this revision. (Show Details)May 14 2020, 9:47 AM
manueljlin edited the summary of this revision. (Show Details)May 14 2020, 10:28 AM
ngraham edited the summary of this revision. (Show Details)May 14 2020, 2:21 PM
ndavis added a subscriber: ndavis.May 14 2020, 2:38 PM

You'll also need to update the index.theme files to include places/48

manueljlin updated this revision to Diff 82861.EditedMay 14 2020, 3:24 PM

Include places/48

edit: on both index.theme files

comments apply to both files

icons-dark/index.theme
105

You also need to add the new folder here

480–481

This should be changed to Fixed

480–481

Delete

481

Delete

487

Scalable -> Fixed

488

Delete

489

Delete

I just noticed, the new auto-generated 24px places icons aren't being used because the places/24 folder isn't in the index.theme files, but that's outside the scope of this patch.

manueljlin updated this revision to Diff 82865.May 14 2020, 3:47 PM
manueljlin marked 7 inline comments as done.

Fixed the issues from the comments

manueljlin updated this revision to Diff 82866.May 14 2020, 4:02 PM

Oops. Actually set 64x64 to fixed and remove MinSize and MaxSize

should line 495 "## Status" actually be removed?

should line 495 "## Status" actually be removed?

No. Phabricator doesn't keep the original context for those comments if you change the code they were referring to. I think GitLab doesn't have that problem.

ndavis added inline comments.May 14 2020, 6:05 PM
icons-dark/index.theme
477

This and the other comments should probably be changed, but I don't actually understand the original purpose anyway.

495

This should be put back the way it was since phabricator mislead you.

manueljlin updated this revision to Diff 82873.May 14 2020, 6:24 PM
manueljlin marked 2 inline comments as done.

Made 64x64 scalable again and tweaked the comments.

Everything should be good now, I think

ngraham accepted this revision.May 14 2020, 6:52 PM

Looks fantastic to me. All good now, @ndavis?

This revision is now accepted and ready to land.May 14 2020, 6:52 PM
ndavis requested changes to this revision.May 14 2020, 7:50 PM

I have to say, I'm very impressed overall. There are a few things I'd like to see changed though:

folder-documents should use a single page, not the copy icon

I suppose they should all have the corner at the top right, but that can be done another time.

folder-network is using a different style

folder-scripts is using a different style

panel elements in user-desktop should be more like the 32px version

The 64px version should be changed too, but that can be done another time.

Other observations (not required, just documenting them so they aren't forgotten)

folder-games inner symbol looks a bit small, but that's going to be extra work, so I won't fault you if you'd rather do that later. The 16px version that you reused for the 48px symbol is a bit too small at 16px anyway, so that can be done another time.

Interestingly, document-print uses a different style for the smallest printer icon size. I think I prefer the document-print version, but that change can be done another time.

This revision now requires changes to proceed.May 14 2020, 7:50 PM
manueljlin updated this revision to Diff 82904.May 15 2020, 8:02 AM

Changed folder-documents, folder-networks, folder-script, user-desktop.

ndavis accepted this revision.May 15 2020, 8:26 AM

LGTM

This revision is now accepted and ready to land.May 15 2020, 8:26 AM
This revision was automatically updated to reflect the committed changes.