file-icons / atom

Atom file-specific icons for improved visual grepping.
MIT License
1.31k stars 250 forks source link

Icons don't show when using City Lights theme #745

Closed MrZyr0 closed 6 years ago

MrZyr0 commented 6 years ago

Hello Before I used seti-ui and seti-syntax with your package and there were no problems but now I've changed for City Lights and the tab icons are no longer displayed ... I don't know if you need more info, tel me ;-)

image

Alhadis commented 6 years ago

👋 Hey mate,

This is clearly an upstream issue, as the city-lights-ui-atom theme uses custom icons (like Seti does). Unlike Seti, City Lights doesn't appear to have any setting for disabling icons.

I suppose I'd better send them a pull request...

Alhadis commented 6 years ago

Strangely, their theme didn't display any tab-icons after I installed it. But I did see the issue you were talking about. Sent them a fix in Yummygum/city-lights-ui-atom#40. =)

noudadrichem commented 6 years ago

@MrZyr0 We do have a dedicated icon package that fits the theme very well. See https://atom.io/packages/city-lights-icons

apm install city-lights-icons

Alhadis commented 6 years ago

@noudadrichem That's not the point, he installed the themes expecting to see this package's icons work, as always.

noudadrichem commented 6 years ago

Wow, ok. I can assume he expected it to work properly, but when you're using two 3rd party packages things can't be 100% perfect on both sides. Just letting @MrZyr0 know we also have a dedicated icon package that fits within the theme. City Lights is a suite of goodies, including the icons. 😊

Alhadis commented 6 years ago

@noudadrichem With all due respect, you should be making consideration for file-icons (or any other popular Atom package for that matter).

It's also unrealistic to encourage users to disable file-icons if they install City Lights, as most users are going to want the best of both worlds. You have 60 icons, we have 842: 108 from MFixx, 192 from DevOpicons, and 542 custom icons collated from years of user requests. We also have heavy-duty JS-based mechanics performing modeline and shebang detection, magic number recognition, and among countless other things that can't be achieved using pure CSS.

noudadrichem commented 6 years ago

Wowow, We're not encouraging anyone to disable any package while using City Lights..? People should install and use all the packages they want, if things aren't working properly together they can make Issues or PRs like you did. That's the point of the whole system. I was just letting him know we also have a dedicated package that suits within the theme.

let me repeat myself: you can't expect to have two independent 3rd party packages working together perfectly smooth. I'm glad you're liking the file-icons package so much, i'll merge your PR to give people 'best of both worlds'.

✌️

Alhadis commented 6 years ago

I'm glad you're liking the file-icons package so much,

@noudadrichem I maintain this package, and it's a maintainer's duty to ensure users get the best experience when installing it. This isn't the first time there's been conflict with a UI theme that uses icons, nor do I predict it to be the last. The fix I submitted is as much for your benefit as it is mine.

noudadrichem commented 6 years ago

All things aside; @MrZyr0 You can update the package. https://atom.io/themes/city-lights-ui It's fixed in https://github.com/Yummygum/city-lights-ui-atom/pull/40

Alhadis commented 6 years ago

Hartelijk bedankt dan. :wink: