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 now icon for now.json #470

Merged
merged 4 commits into from
Jul 27, 2019
Merged

Add now icon for now.json #470

merged 4 commits into from
Jul 27, 2019

Conversation

LukasPolak
Copy link
Contributor

No description provided.

@PKief PKief self-requested a review June 13, 2019 22:22
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.

Hi, there's already another pull request for this kind of icon: #428
The problem of your suggested icon is the white color. Could you imagine how it looks like on a light theme with a bright background? I assure you, it'll be hard to recognize. The Material Icon Theme was implemented to support both light and dark backgrounds. This is why we should either provide an icon with a color that works for both themes or two icons, one for light and another for a dark background.

@LukasPolak
Copy link
Contributor Author

Hi, I have added the light version of the icon.
In term of duplicate PR, how should I make the update?
Thanks!

@LukasPolak
Copy link
Contributor Author

It is ok if icons are purely white and black?
It's because of the brand.

@PKief
Copy link
Member

PKief commented Jun 14, 2019

@LukasPolak Thank you, I'll take a look.

It's actually not a problem if multiple pull requests are created for the same icon. I just wanted to point out that there already existed another one ;)

src/icons/fileIcons.ts Outdated Show resolved Hide resolved
src/icons/fileIcons.ts Outdated Show resolved Hide resolved
@PKief PKief changed the title add now icon for now.json Add now icon for now.json Jul 7, 2019
@shreyasminocha shreyasminocha mentioned this pull request Jul 26, 2019
@PKief PKief merged commit bd3e2c6 into material-extensions:master Jul 27, 2019
PKief added a commit that referenced this pull request Jul 27, 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.

None yet

2 participants