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 the Flash icon for .swf, .swc and .sol #200

Merged
merged 2 commits into from
Aug 27, 2016
Merged

Add the Flash icon for .swf, .swc and .sol #200

merged 2 commits into from
Aug 27, 2016

Conversation

Gama11
Copy link
Contributor

@Gama11 Gama11 commented Aug 27, 2016

.sol is a local shared object, aka flash cookie

Didn't update icons.zip to avoid merge conflicts with the other PRs.

.sol is a local shared object, aka flash cookie
@robertohuertasm
Copy link
Member

It's ok but I would rather make the icon smaller. Let's put some space between the border and the icon. Let's standarize it in 2 pixels of margin as per some other icons in the extension. #193

@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

@robertohuertasm Okey.

@robertohuertasm robertohuertasm mentioned this pull request Aug 27, 2016
11 tasks
@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

The HaxeDevelop icon I added also didn't have any margins I think. Neither did Lime really (#182), but I don't think it's necessary there?

@robertohuertasm
Copy link
Member

I don't think so, don't worry about them. In fact, your lime icon had more than 2px margin although only vertical. It's not a common rule because most of the icons that we currently have are not equal but just to make the icons feel more or less right with the others that we have. If we're making a square icon full 32px it's going to be seen as bigger. It's more a perceptive issue than numeric. @jens1o @Gama11

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

I have actually noticed that the HaxeDevelop icon appears bigger than the others, so I'd like to change that one.

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

Done!

@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

You need to also add them to the icons.zip.

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

As mentioned previously:

Didn't update icons.zip to avoid merge conflicts with the other PRs.

@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

Oh, I overlooked this notice. But you could create a new branch, in which you update the zip file.

For every PR, I create a different branch.

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

What would that do?

@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

You can create a new branch, which does not affect the other files. Git just add the binaries, so you can apply multiple patches to each other.

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

I'm well aware of that. I'm actually trying to avoid creating merge conflicts with your PRs, not my own.

@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

I thought it just add or remove them? In a zip, it would add them, don't it?

@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

Hm.. I thought Git might just treat .zip as a binary file. Maybe not!

@robertohuertasm
Copy link
Member

@Gama11, @jens1o I'm not really sure about this either so I've changed the code and now icons.zip is just a build artifact. Every time anyone runs build task icons.zip file will be created from icons folder. 😁

@robertohuertasm robertohuertasm merged commit 506ab40 into vscode-icons:master Aug 27, 2016
@jens1o
Copy link
Member

jens1o commented Aug 27, 2016

Nice thing! ❤️

@Gama11 Gama11 deleted the flash branch August 27, 2016 17:19
@Gama11
Copy link
Contributor Author

Gama11 commented Aug 27, 2016

Awesome!

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

3 participants