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

dark menu bar icons prop #1258

Closed
wants to merge 1 commit into from
Closed

dark menu bar icons prop #1258

wants to merge 1 commit into from

Conversation

charlesrocket
Copy link

@charlesrocket charlesrocket commented Jan 23, 2018

Dark tray icon set suggestion

bisq1
bisq2

outlined cat version doesn't look acceptable with non-retina displays, so i deiced to test with these instead

@ripcurlx
Copy link
Contributor

I think inverting it like that is the the better way, as it would get too fine grained on dark background using just the outline. I think I will have to look at the tray icon for light mode as well, as it has this grey shadow underneath the cat. So it doesn't look perfect when selected in light theme atm.
tray icon

@ManfredKarrer
Copy link
Contributor

That would require code changes also to support dark themes. Easier fix is to use a white background.

@ManfredKarrer
Copy link
Contributor

I will close the PR for now. Feel free to reopen it after the icon support would get full support and has been tested.

@cbeams
Copy link
Contributor

cbeams commented Jan 31, 2018

I've added the was:incorrect label, indicating that this PR was not a 'correct' PR in the sense that it was incomplete, untested, didn't have code actually to make the change that was suggested.

Nothing wrong here, by the way @charlesrocket—thanks for submitting. We're just in the process here of getting really good at letting folks know what's required for a PR to be "correct" and thus to get merged, so this is helpful in the sense that it provides an example of an "incorrect" PR.

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

Successfully merging this pull request may close these issues.

4 participants