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

Adding new folder icons bis #346

Conversation

AdrieanKhisbe
Copy link
Contributor

Follow up of #339, A new serie of icons. 🎨

  • Add two new folder icon e2e for end to end test and custom 95e6f7e
  • Singularize screenshot and icon folder icons from images 92954c6
  • Add icon for grunt folder cdde91d
  • Add the forgotten docker-compose.test.yml to the dockers file e1de3ba

I've some other icons in the go, there are still are research stage

@AdrieanKhisbe
Copy link
Contributor Author

Here are the other icons I have in mind:
Feedback are more than welcome!! :)

@AdrieanKhisbe
Copy link
Contributor Author

Also Pimped up NodeJs icon to please a collegue
capture d ecran 2018-11-20 09 56 33

@PKief
Copy link
Member

PKief commented Nov 20, 2018

Wow, very cool! I very appreciate your work! At a first glance the icons look very good, but I still have to review them in more detail 👍


I'd like to give you another tip to make the folder icons even better. Usually I make the smaller front icon overlap slightly over the right and bottom edges of the folder icon:

YoursImproved
image 1 image 2

I tried to make this for each folder icon in the theme as far as possible to get a consistent look.

It'd be great if you could improve that a little bit for your icons.

But thank you very much, you did a great job (again) 😄

@AdrieanKhisbe
Copy link
Contributor Author

Cool!
About icon overlapping, you're right, I really have overlook it for this one. And maybe some.
I'll ping you when I'll have them fix, and the other ones mentioned added.

😃

@AdrieanKhisbe
Copy link
Contributor Author

Et voilà. I complete the folder icons, and tweaked the one that need to be done.
Here is a preview:
capture d ecran 2018-11-21 00 47 48

@PKief
Copy link
Member

PKief commented Nov 21, 2018

@AdrieanKhisbe Looks great 😃. I'll review it soon.

@AdrieanKhisbe
Copy link
Contributor Author

👌 =)

@PKief PKief force-pushed the master branch 3 times, most recently from 72a1988 to 0fcc978 Compare November 21, 2018 21:27
{ name: 'folder-scripts', folderNames: ['script', 'scripts'] },
{ name: 'folder-node', folderNames: ['node_modules'] },
{ name: 'folder-node', folderNames: ['node_modules', 'npm'] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node !== npm

I think the node folder icon does not fit very good to the npm folder. And I haven't seen so many npm folders before, not sure if we need an icon for that folder at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right

@@ -10,18 +10,17 @@ export const folderIcons: FolderTheme[] = [
rootFolder: { name: 'folder-root' },
icons: [
{ name: 'folder-src', folderNames: ['src', 'source', 'sources'] },
{ name: 'folder-dist', folderNames: ['dist', 'out', 'build', 'release', 'bin'] },
{ name: 'folder-dist', folderNames: ['dist', 'out', 'build', 'release', 'bin', 'ltximg', 'latex.out'] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two folder names are very specific, not sure if I'll add them to the theme. If people want these icons they can assign them manually in the user settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, I forgot this was possible. =)
(just found it in the readme: https://github.com/PKief/vscode-material-icon-theme#folder-associations)

@AdrieanKhisbe
Copy link
Contributor Author

@PKief I just applied your recommendations =)

@PKief
Copy link
Member

PKief commented Nov 27, 2018

Alright, I spend some time to review the icons and also to create some new icons. Finally these icons will be added to the theme:

new folder icons

Added folder icons:
- animation
- review
- rules
- syntax

Updated folder icons:
- coverage (updated color)
- custom (pencil is more intuitive)
- flow (updated color)
- mock (pencil is more intuitive)
- stylus (updated color)
- vm (updated color)

Rejected folder icons:
- e2e (uses coverage icon now)
- grunt (not suitable)
- icon (uses the images icon, does not need another icon)
- node (current icon is nice)
- scenario (not common and not so suitable)
- screenshot (images folder is enough for that, a screenshot folder still contains images)
- storybook (not suitable -> see #235 (comment))
- styleguide (not common and not so suitable)
- idea (not suitable)

Updated file icons:
- virtual (another icon)
- stylus (small improvement)


Further explanations:
"not suitable" means that it does not really fit to the other icons in the theme. It has nothing to do with your work, I couldn't do any better (I even tried to work on each of them). But it is better to omit some icons than that the quality of the theme suffers from it.

In my opinion, the folder icons "Screenshot" and "Icons" can definitely be replaced by the default image folder icon, I don't see any real enrichment here.

icons

I made the "vm" folder icon blue because of the existing file icon for virtual box files (*.vbox):

vm

"e2e" and the existing "coverage" icon looked very similar, which is why I use the already existing coverage icon and adjusted the color a little bit.

compare e2e and coverage icons

For the "mock" and "custom" folder icons I decided to use a pencil as the motive and another color for "mock" because I think this is more intuitive:

YoursNew version
image 1 image 2

To note this again, I really appreciate your work and the icons are great and you've great ideas. If I decide not to use an icon, it doesn't mean that you did a bad job, but that the icon doesn't fit properly. I hope that you understand my decisions 😅

Ok so far so good 👍
Thank you very much!

@PKief
Copy link
Member

PKief commented Nov 27, 2018

Merged with this commit cfec8f1.

@PKief PKief closed this Nov 27, 2018
@AdrieanKhisbe
Copy link
Contributor Author

AdrieanKhisbe commented Nov 28, 2018

Hey @PKief,
Just seeing now your message.
No problem with some icons being not take, I do understand your position and agree some were not "suitable", and though to me screenshot&icons looked nice, we can do for sure with just the pictures.
Your additions are great, and the mock+custom are way better than my original design.


I'm almost settle with the folders icons, just remain a last icon to crack, the .idea folder which can be very common.
Have you some ideas?

I was thinking either just keeping a bordered squared with ID.
Or doing a more generic editor icon that we could use for other editors folder like atom.
With an icon like: https://material.io/tools/icons/?icon=format_size&style=baseline

@PKief
Copy link
Member

PKief commented Nov 30, 2018

Yes, the .idea folder is very common. Maybe something like this could be possible:

.idea folder icon

But I'm still not very sure, I've to think about it. Let's see.

I'm against a generic folder icon for such folders, then I'd rather have none ;)

@AdrieanKhisbe
Copy link
Contributor Author

That icon folder looks very nice!

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