D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-15 Thread Phabricator 
This revision was automatically updated to reflect the committed changes.
Closed by commit R266:010c34f0e7c5: Add enablefont and disablefont icon for 
kfontinst KCM (authored by Guo Yunhe i...@guoyunhe.me).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66083=66100

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/16/font-disable.svg
  icons-dark/actions/16/font-enable.svg
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/16/font-disable.svg
  icons/actions/16/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Noah Davis 
ndavis accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Nathaniel Graham 
ngraham added a dependent revision: D23868: [kfontinst] Port to use icons from 
the icon theme.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Nathaniel Graham 
ngraham added a comment.


  I still feel like I'd prefer the checkmark to be green. I also feel like I 
understand the argument that red isn't the best color to use for disabling 
something since it isn't destructive. Maybe we should use the orange color for 
that?
  
  Definitely need clarification in the HIG.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  @ndavis can you give review? thanks!

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 accepted this revision.
GB_2 added a comment.


  Now it's good!

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  Changed to `currentColor`

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66083.
guoyunhe added a comment.


  Remove hardcoded colors

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66080=66083

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/16/font-disable.svg
  icons-dark/actions/16/font-enable.svg
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/16/font-disable.svg
  icons/actions/16/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 added a comment.


  In D23942#531364 , @guoyunhe wrote:
  
  > @GB_2 is it correct now?
  
  
  There's still a hardcoded `style="fill:#da4453"` in the `disable` icons.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  @GB_2 is it correct now?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66080.
guoyunhe added a comment.


  Add inline styles

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66077=66080

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/16/font-disable.svg
  icons-dark/actions/16/font-enable.svg
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/16/font-disable.svg
  icons/actions/16/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 added a comment.


  In D23942#531349 , @guoyunhe wrote:
  
  > I was asked to remove the hard code color like: `fill="currentColor"` and 
`fill="#da4453"`. But I see other icons have something like 
`style="fill:currentColor"`. Can anyone tell me correct way of applying colors?
  
  
  Just `fill="currentColor"` is correct.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  I was asked to remove the hard code color like: `fill="currentColor"` and 
`fill="#da4453"`. But I see other icons have something like 
`style="fill:currentColor"`. Can anyone tell me correct way of applying colors?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 added a comment.


  In D23942#531347 , @guoyunhe wrote:
  
  > @ndavis it looks strange when I removed the inline style
  >
  >   {F7351830}
  
  
  The inline style needs to be included, how it is now.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  @ndavis it looks strange when I removed the inline style
  
  F7351830: image.png 

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 accepted this revision.
GB_2 added a comment.


  Then it's fine.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis, GB_2
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  I just followed the email icons' color: check mark is black/white, disable 
mark is red.
  
  F7351796: image.png 

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Noah Davis 
ndavis added a comment.


  In D23942#531337 , @GB_2 wrote:
  
  > In D23942#531330 , @ngraham 
wrote:
  >
  > > Shouldn't the checkmark be green?
  >
  >
  > FWIW, both should be black/white. Disabling is not a destructive action 
like removing.
  
  
  We use red for disabling as well. Perhaps we need to work on this a bit. One 
of the problems with only using black and white is that the emblems become 
harder to notice.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Björn Feber 
GB_2 added a comment.


  In D23942#531330 , @ngraham wrote:
  
  > Shouldn't the checkmark be green?
  
  
  FWIW, both should be black/white. Disabling is not a destructive action like 
removing.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: GB_2, ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Nathaniel Graham 
ngraham added a comment.


  Shouldn't the checkmark be green?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66077.
guoyunhe added a comment.


  Remove hardcoded colors

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66071=66077

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/16/font-disable.svg
  icons-dark/actions/16/font-enable.svg
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/16/font-disable.svg
  icons/actions/16/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Noah Davis 
ndavis added a comment.


  In D23942#531279 , @guoyunhe wrote:
  
  > @ndavis, @broulik, @ngraham, I made some change. Can you have another look?
  
  
  I just noticed that the "No" symbol is backwards. Once you fix that and the 
hardcoded colors, this patch will be ready to land.

INLINE COMMENTS

> font-disable.svg:11
> + fill="currentColor"/>
> +
> +

Hardcoded color

> font-disable.svg:10
> +
> + fill="#da4453"/>
> +

Hardcoded color

> font-disable.svg:11
> + fill="currentColor"/>
> +
> +

Hardcoded color

> font-disable.svg:10
> +
> + fill="#da4453"/>
> +

Hardcoded color

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  @ndavis, @broulik, @ngraham, I made some change. Can you have another look?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66071.
guoyunhe added a comment.


  Add 16px icons for font-enable and font-disable

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66067=66071

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/16/font-disable.svg
  icons-dark/actions/16/font-enable.svg
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/16/font-disable.svg
  icons/actions/16/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66067.
guoyunhe added a comment.


  Reduce space between A and marks

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66050=66067

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe updated this revision to Diff 66050.
guoyunhe edited the summary of this revision.
guoyunhe added a comment.


  Rename enablefont and disablefont icons

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23942?vs=66043=66050

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/22/font-disable.svg
  icons-dark/actions/22/font-enable.svg
  icons/actions/22/font-disable.svg
  icons/actions/22/font-enable.svg

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Noah Davis 
ndavis added a comment.


  Oof, all of the font icons that contain an 'A' need to be cleaned up someday. 
The 'A's aren't even 16px tall and the line thickness and alignment is all over 
the place. I guess that's something for another patch. For now, we should 
maintain consistency with the existing font icons.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe added a comment.


  In D23942#531086 , @broulik wrote:
  
  > Please use a more hierarchical name, e.g. "font-enable" and "font-disable", 
to match the other font icons we have such as "font-size-up" and "font-face"
  
  
  OK. Then need to update kfontinst, too.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Noah Davis 
ndavis requested changes to this revision.
ndavis added a comment.
This revision now requires changes to proceed.


  There's a bit too much space around the checkmark.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze, ndavis
Cc: ndavis, broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Kai Uwe Broulik 
broulik added a comment.


  Please use a more hierarchical name, e.g. "font-enable" and "font-disable", 
to match the other font icons we have such as "font-size-up" and "font-face"

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze
Cc: broulik, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe edited the summary of this revision.
guoyunhe added a reviewer: Breeze.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D23942

To: guoyunhe, #breeze
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23942: Add enablefont and disablefont icon for kfontinst KCM

2019-09-14 Thread Yunhe Guo 
guoyunhe created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
guoyunhe requested review of this revision.

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23942

AFFECTED FILES
  icons-dark/actions/22/disablefont.svg
  icons-dark/actions/22/enablefont.svg
  icons/actions/22/disablefont.svg
  icons/actions/22/enablefont.svg

To: guoyunhe
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns