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

Writefull and LayoutEditor (3D and TextEdit) #3580

Closed
wants to merge 2 commits into from

Conversation

leoheck
Copy link
Contributor

@leoheck leoheck commented Feb 4, 2017

Pull request for Writefull.svg

Using Inkscape 0.92. Starting from an existing icon. I selected the ignore option at this time. What do you recommend to do for the next time? This makes any difference for the final product?
image

@leoheck leoheck mentioned this pull request Feb 4, 2017
@leoheck leoheck changed the title Adding initial version for Writefull icon Writefull and LayoutEditor (3D and TextEdit) Feb 4, 2017
@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

Also addressing this issue #3571 (comment)

@bilelmoussaoui
Copy link
Contributor

@leoheck Please separate each icon in a different PR

@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

Ok. Can you explain how to do that?

@leoheck leoheck closed this Feb 4, 2017
@bilelmoussaoui
Copy link
Contributor

All you have to do is to create separate branch for each icon. For now all the icons you have added are on master(on your fork). Delete all of them from master. Create a branch for each icon, and commit to that branch instead of master. Once you've done go to your Github fork, and choose each branch, compare it with origin (the Numix core) and create a PR. Don't forget to add the icon entry on the json data file 👍

@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

Good @bil-elmoussaoui, can I put the 2 icons related with layout editor in the same branch?

@bilelmoussaoui
Copy link
Contributor

It's better to keep each icon in a separate branch/PR. @palob What do you think about that?

@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

Oh, I just put them in the same PR, sorry. I didn't know these rules. @palob should I change this or is it fine this time?

@palob
Copy link
Member

palob commented Feb 4, 2017

Better one per PR, now that we need to keep track of data.json an both Circle+Square.
Both layouteditor icons may go into one, might help with keeping the design in line.

About the Inkscape popup, I haven't investigated thoroughly (0.92.1 will adress it) but Ignore is what I go with. Our graphics aren't for printing.

@bilelmoussaoui
Copy link
Contributor

@leoheck Also, both of your PR doesn't include a square icon, can you get that fixed ?

@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

Sure. So I have to give both circle and squared? It will be nice to have these instructions in readme/wiki so you just need to point that for the next time.

Some section like: Pull Requests Rules:

  • Create a single icon for each pull request
  • Add the icon in data.json
  • Add both circle and squared version.

What do you think?

Also, what about giving just the icon emblem, and a custom script to generate the circle/square bg.
Or giving the circle icon and generating the squared version and vice-versa.

Also, it will be nice to have an icon template to start with.

@bilelmoussaoui
Copy link
Contributor

For the templates, you can get them from here https://github.com/numixproject/numix-core/tree/master/templates, for the rest @Foggalong will give you a better answer!

@palob
Copy link
Member

palob commented Feb 4, 2017

Yeah, we need to write a thorough icon design guide some time.

@Foggalong Foggalong added the merge label Feb 4, 2017
@leoheck
Copy link
Contributor Author

leoheck commented Feb 4, 2017

This will be great because anyone can help without being rejected every time. Sometimes good people just lose the will as they discover the rules after each modification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants