Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config for grayscale icons #387

Merged
merged 2 commits into from
Feb 13, 2019
Merged

Conversation

cezarsa
Copy link
Contributor

@cezarsa cezarsa commented Feb 7, 2019

Hi, I'm not a big fan of colored icons as I think they draw too much attention, however, I do like having different symbols for each file type.

Inspired by the awesome hack used to change the icons opacity I added a new command and configuration to allow switching all icons to grayscale. This is done by adding an SVG filter to each icon file changing its saturation to 0.

Here is an example of how it looks: (I loved it!)

@PKief
Copy link
Member

PKief commented Feb 9, 2019

Hey @cezarsa, this is one of the best PRs I've ever received 😄
It has such a good quality that it will be definitely merged 👍

I also like the idea of toggling the greyscale filter on and off. It would be interesting for me if you could give me some feedback if we should make it "true/false" as in this PR or provide an option (same as for the opacity) to customize the saturation by value:

Saturation 1 Saturation 0.5 Saturation 0.3 Saturation 0
saturation 1 saturation 0.5 saturation 0.5 saturation 0

Do you think it makes sense to customize the saturation?

@PKief PKief self-assigned this Feb 9, 2019
@PKief PKief self-requested a review February 9, 2019 19:53
@mallowigi
Copy link

This feature looks great! I think I’ll implement it as well :)

@cezarsa
Copy link
Contributor Author

cezarsa commented Feb 11, 2019

@PKief Thank you. :) I considered exposing the saturation directly but in the end I thought having only a grayscale config would be more straightforward for end users.

However, I do like the flexibility of tweaking with different saturations. I think a compromise may be reached by having a single configuration option configuration.saturation as a float and 2 different commands:

command.grayscale would still exist and set the saturation config to 0 or 1 by enabling or disabling it;
command.saturation would read a float value similar to the opacity command and set the saturation directly.
What do you think about this approach? I can update this PR with this changes.

@PKief
Copy link
Member

PKief commented Feb 11, 2019

@cezarsa Yes, this sounds really good 👍
I also think it's better to have the saturation as float value in the settings and do the rest via commands.
It would be great if you could change the PR to this. If you might need any help, don't hesitate to ask! 😃

@cezarsa
Copy link
Contributor Author

cezarsa commented Feb 12, 2019

It's done, please take a look.

Copy link
Member

@PKief PKief left a comment

Choose a reason for hiding this comment

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

Thank you very much, it looks very good and has a very high quality.

@PKief PKief merged commit 77dc451 into material-extensions:master Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants