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 support for SVG icons #205

Closed
wants to merge 1 commit into from
Closed

Added support for SVG icons #205

wants to merge 1 commit into from

Conversation

theenadayalank
Copy link

Now we can able to use SVG icons instead of font icons.

@mansona
Copy link
Owner

mansona commented Apr 19, 2018

@theenadayalank thanks for the PR 🎉

I have laid out a vision for this addon in this comment #185 (comment) but I thought I would respond to your PR directly.

I want this PR to land (eventually). It's currently not the best experience for devs when they install ember-cli-notifications and it doesn't work out of the box because you need font-awesome configuration. I want to move to SVG as the default with font-awesome or bootstrap as optional.

There are a few points on on the PR and how we might need to proceed:

  • making this the default will require a major version bump. I have no objection (integers are cheap) but we might batch it together with a few other things that we want to move around at a major bump
  • Where did you get these Icons? I am happy to use them but we need to make sure the copyright is ok 👍 also can you share an image of what this looks like as a comparison please
  • I am not likely to add inline-svg as a dependency as you can use SVG directly in Ember. I would recommend this awesome talk by Jen Weber that taught me how to do SVG aweseomely in Ember 💪

I hope this is useful in some way 😖

@theenadayalank
Copy link
Author

Thank you @mansona for your great suggestions🙂. Sorry for the delay in getting back to you.

The talk by Jen Weber about inlining SVG in ember gave me a lot more insights to handle SVGs😉. The icons I used are downloaded from material.io . Yes, they are open sourced icons.

I will change the code as per the suggestions you have given and will let you know once I finish the changes💪.

@theenadayalank
Copy link
Author

theenadayalank commented Jun 23, 2018

I have pushed my changes. Kindly review the PR and do the needful 🙂
@ynnoj

@mansona
Copy link
Owner

mansona commented Nov 12, 2019

Hey @theenadayalank, me again 😂

Sorry for the delay but I have actually included your change in #251 (comment) and actually made it the default and only icons for this addon 😱

I also cherry-picked your change so you got the credit in the PR 🎉 thanks again for your contribution and sorry for taking so long to get it sorted

@mansona mansona closed this Nov 12, 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.

2 participants