Maddoc42 / Android-Material-Icon-Generator

Android icons with looooong material shadows!
https://android-material-icon-generator.bitdroid.de/
Other
770 stars 66 forks source link

Add banner #7

Closed kornfleks closed 8 years ago

kornfleks commented 8 years ago

Hello :),

Just add "beta" banner. In the future I will add "dev" & "alpha" banner, and fix the position.

Maddoc42 commented 8 years ago

Two general things:

More specific to your changes: I like the general idea of adding a short side banner to the icon. Here are some things that would have to happen though before I will publish those changes on the main branch:

Does this sound reasonable?

That being said I won't merge this into master now, but I am willing to keep reviewing changes and help where I can :)

kornfleks commented 8 years ago

Yes you are right I have forgottent Banner.js (but the test fail again, why ?). I will do a new pull request when your request features (Goods ideas :+1:) will be added.

I'm motivated by this project, great job ! (My english isn't perfect, sorry)

kornfleks commented 8 years ago

Can you give me some tips, I don't know why my code don't pass the tests :/.

Maddoc42 commented 8 years ago

Glad to hear you're motivated!

Just did a clone of your changes, have you tried exporting the icons locally? That long works which is also the reason why the test cases fail.

In your first commit you (presumably accidentally) removed the following line from the editor.static.jade

canvas#canvas-export-playstore-icon

That cause the error you are seeing.

kornfleks commented 8 years ago

So I add this features :

The collapse sub menu isn't perfect, but I don't want touch too much of your css right now. Perhaps you will find a good looking solution based on my work.

So I have some other ideas :

PS: thanks for your help with your code, I have tried to follow your tips best I can :+1:

Maddoc42 commented 8 years ago

Nice, it's starting to come along! Couple of things I noticed:

In addition here are some more suggestions:

Please feel free to change the css. I'm pretty new to it, so there a probably lot's of ways to make it better.

Thanks for all the hard work so far!

kornfleks commented 8 years ago

The test failed because of a 403 error ^^.

Your six demands has been granted :). I see only one bad behaviour, but I don't really know how to fix it properly. When you expand the banner sub-menu the scroll-bar doesn't go to the bottom :/.

In addition, the mouse wheel, to scroll the menu, isn't supported.

Maddoc42 commented 8 years ago

Awesome :) thanks for the pull request. I'll look into the remaining issues and publish the result once those are fixed. Really great work!

kornfleks commented 8 years ago

Thanks you ! It's a pleasure to work with you :+1:

Maddoc42 commented 8 years ago

Changes are live now. Banner looks really great. Thanks again :)

ic_launcher