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

feat: Adding new icons #554

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Brijeshkrishna
Copy link
Contributor

@Brijeshkrishna Brijeshkrishna commented Oct 22, 2023

Changed icons

CONFIG from e615
Screenshot_20231022_210120
to f107b
Screenshot_20231022_210207
f107b shows it is a file (config file).


FILE_OUTLINE from f016
Screenshot_20231022_210651
to f086f
Screenshot_20231022_210707
question mark makes easy to identify it is a unknown file (no file extension).


GIT from f1d3
Screenshot_20231022_210326
to f02a2
Screenshot_20231022_210349
f1d3 show git in the icon , f02a2 shows the logo in icon.

OLD

Screenshot_20231022_212536

NEW

Screenshot_20231022_212558

New Folder icons

Screenshot_20231022_211119

@PThorpe92
Copy link
Member

PThorpe92 commented Oct 22, 2023

Thanks for making the changes easy to see, and for adding the tests.. Personally I like near enough all of those changes. Icons are certainly a matter of preference so we'll see what the rest of the maintainers think, but I like it 👍

@Brijeshkrishna
Copy link
Contributor Author

New icons+

Screenshot_20231023_122637

@cafkafk
Copy link
Member

cafkafk commented Oct 24, 2023

New icons+

Screenshot_20231023_122637

I'm not sure if I'm for or against these, but I personally associate the hashtag with code, so perhaps this should be reserved for code folders (like src, build, benches, tests)

@Brijeshkrishna
Copy link
Contributor Author

Since src, build, and test are different,So I used different icons for them.

src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
@Brijeshkrishna
Copy link
Contributor Author

@cfxegbert
Screenshot_20231103_213220
This icon for test folder is the better choice because wrench represents testing, instead of build folder.

@cfxegbert
Copy link
Contributor

@cfxegbert Screenshot_20231103_213220 This icon for test folder is the better choice because wrench represents testing, instead of build folder.

We use the wrench icon to represent files such as Makefile that build other files and not for testing.

src/output/icons.rs Outdated Show resolved Hide resolved
src/output/icons.rs Outdated Show resolved Hide resolved
@Brijeshkrishna
Copy link
Contributor Author

Screenshot_20231122_233342

Suggested by @hasecilu

@PThorpe92
Copy link
Member

Could you do me a favor and add 󰘧 for .opam files ? ( nf-md-lambda )

I forgot this in a previous PR and don't want to open a new one just for one icon.

@Brijeshkrishna
Copy link
Contributor Author

Could you do me a favor and add 󰘧 for .opam files ? ( nf-md-lambda )

I forgot this in a previous PR and don't want to open a new one just for one icon.

.opam is for OCaml.

@PThorpe92
Copy link
Member

.opam is for OCaml.

Yes I am well aware..

We already have  for .ml and dune files, so I wanted to have the lambda icon for .opam files just so it's not all the same. Many editors use the lambda icon for ocaml because .ml is obv meta-language and not ocaml specific

@Brijeshkrishna
Copy link
Contributor Author

@cfxegbert any changes ?

src/output/icons.rs Outdated Show resolved Hide resolved
@Brijeshkrishna
Copy link
Contributor Author

@cafkafk @cfxegbert, any changes ?

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Could you fix the conflicts, then I'll take another look :)

@Brijeshkrishna
Copy link
Contributor Author

Could you fix the conflicts, then I'll take another look :)

Done 👍

@Brijeshkrishna
Copy link
Contributor Author

@cafkafk ?

@Brijeshkrishna
Copy link
Contributor Author

@cafkafk @PThorpe92 @cfxegbert ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants