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

Added pwa folder icons #939

Merged
merged 2 commits into from
Jan 25, 2021
Merged

Added pwa folder icons #939

merged 2 commits into from
Jan 25, 2021

Conversation

andylib93
Copy link
Contributor

No description provided.

@andylib93
Copy link
Contributor Author

The link for the icon colors does not work

https://material.io/design/style/color.html#color-color-palette

@PKief
Copy link
Member

PKief commented Jan 24, 2021

Thanks, I updated the link :)

https://material.io/design/color/the-color-system.html#tools-for-picking-colors

@PKief
Copy link
Member

PKief commented Jan 24, 2021

You can also you this tool:
https://pkief.github.io/material-color-converter/

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.

I think it would be more intuitive if we had a self-speaking icon instead of writing "PWA". So as a suggestion we could resuse the "client" folder icon for names like "pwa". What do you think about that?

If you like you can check how it looks like with the following VS Code user setting:

"material-icon-theme.folders.associations": {
    "pwa": "client
}

If it looks good you could update this PR with these changes. Otherwise we could also look for a complete new icon for this. But I think that the "client" folder icon could fit very well in this case.

@andylib93
Copy link
Contributor Author

Oh this makes sense to use the client icon folder for a 'pwa-folder'. My inspiration came from this repo:

https://github.com/webmaxru/progressive-web-apps-logo

Given the fact that PWAs have their own logo, I thought I would add it like this. For example I currently develop a PWA for a client and I just put all the content inside a simple 'pwa' folder and noticed that there is no icon folder for it yet.

What do you think?

@andylib93
Copy link
Contributor Author

Btw your color converter is the bomb, I urge you to add it in the contributing guidelines :)

@PKief
Copy link
Member

PKief commented Jan 24, 2021

Oh this makes sense to use the client icon folder for a 'pwa-folder'. My inspiration came from this repo:

https://github.com/webmaxru/progressive-web-apps-logo

Given the fact that PWAs have their own logo, I thought I would add it like this. For example I currently develop a PWA for a client and I just put all the content inside a simple 'pwa' folder and noticed that there is no icon folder for it yet.

What do you think?

I understand the reason why you chose this icon. In my opinion it's not so suited to be used in this icon extension. I'd prefer using an icon like "client" which is more intuitive. I think a symbol is so much more intuitive than some letters.

@PKief
Copy link
Member

PKief commented Jan 24, 2021

Btw your color converter is the bomb, I urge you to add it in the contributing guidelines :)

Really appreciate your feedback :) I added it to the contributing, too. Thanks for the hint.

@andylib93
Copy link
Contributor Author

I understand! Given that this is my first PR ever, will you just close this PR and change the PWA option or how would you like to manage it?

@PKief
Copy link
Member

PKief commented Jan 24, 2021

No worries, I can update your PR, so that the contribution is still yours ;)

@PKief PKief self-requested a review January 24, 2021 21:40
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.

I updated the PR! Will now be merged!

@PKief PKief merged commit e7120a3 into material-extensions:master Jan 25, 2021
@andylib93
Copy link
Contributor Author

I updated the PR! Will now be merged!

Thank you 😊

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