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

macOS: Use electron API to get dark tray icon for the light theme. #941

Merged
merged 1 commit into from
May 19, 2020

Conversation

manavmehta
Copy link
Collaborator

@manavmehta manavmehta commented Apr 24, 2020

Context: #861

What's this PR do?
We made a change to make the tray icon look flushed and coherent for the dark theme
A corresponding dark icon would do the necessary for the light theme
Electron makes it possible by using Template Image

Screenshots?
Presently we have this
old light theme
dark theme

This commit makes this change
new light theme
dark theme

You have tested this PR on:

  • macOS Catalina 10.15.2

@manavmehta
Copy link
Collaborator Author

@akashnimare if you could test it on your mac

@akashnimare
Copy link
Member

@manavmehta -

https://cl.ly/4d37215646da

Check if electron handles this by default otherwise we need to add IPC when user updates the theme.

@manavmehta
Copy link
Collaborator Author

@akashnimare please have a look.
Electron is a beauty 😄

@manavmehta manavmehta changed the title macOS: Add a coherent dark tray icon for the light theme on macOS macOS: Use electron template image to get dark tray icon for the light theme Apr 27, 2020
@akashnimare
Copy link
Member

@manavmehta please fix merge conflicts.

@manavmehta
Copy link
Collaborator Author

@akashnimare fixed them.
Also was this comment intended to be here?

@akashnimare
Copy link
Member

Did you update the tray image? I think it was updated in #894.

@manavmehta
Copy link
Collaborator Author

manavmehta commented May 2, 2020

@akashnimare Afaik, #894 only updates the dock icon, not the tray icon
What this PR does: electron uses Template Image which consists of a black and an alpha channel and can adjust its appearance based on the theme

Quoting from electron docs :

The most common case is to use template images for a menu bar icon, so it can adapt to both light and dark menu bars.

So, we don't need two images and any ipc calls to change the image, electron does this by itself
I just renamed the tray image to take advantage of that and we won't need the green icon - coherence

@akashnimare
Copy link
Member

@manavmehta fix the merge conflicts and we can merge this. As a follow-up, we should update our tray image because the white padding is clearly visible on macOS.

@manavmehta
Copy link
Collaborator Author

@akashnimare resolved.
Also, would it be good to open an issue for this? (as there is less chance of missing out on an issue if it's listed)

we should update our tray image because the white padding is clearly visible on macOS.

app/renderer/js/tray.ts Outdated Show resolved Hide resolved
app/renderer/js/tray.ts Outdated Show resolved Hide resolved
@kanishk98
Copy link
Collaborator

As a follow-up, we should update our tray image because the white padding is clearly visible on macOS.

Hmm, just going through this issue and I'm not sure I follow this. @akashnimare could you clarify what you mean by white padding?

…acOS

The white tray icon for dark theme looks coherent with other icons
A corresponding dark icon for the light theme would make it more flushed into the UI
@akashnimare
Copy link
Member

LGTM on macOS.

@akashnimare akashnimare changed the title macOS: Use electron template image to get dark tray icon for the light theme macOS: Use electron API to get dark tray icon for the light theme. May 19, 2020
@akashnimare akashnimare merged commit 4f890c0 into zulip:master May 19, 2020
@akashnimare
Copy link
Member

Merged. Thanks, everyone!

@manavmehta manavmehta deleted the dark-tray-icon branch June 25, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants