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

Add option to always highlight the tray icon #6620

Merged
merged 4 commits into from Jul 27, 2016

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Jul 26, 2016

For applications showing "popover" style windows when the tray icon is clicked, it is nice to highlight the tray icon background for the entirety of when the window is open (like screenhero, dropbox, 1password, etc. do).

Previously the icon would highlight only when clicked but the highlight would go away on mouse up.

Now you can control when the highlight goes away explicitly so you can maintain the highlighted background until your window is closed.

Since there was already a tray.setHighlightMode(mode) API, I expanded that API to take a string enum instead of a boolean.

The possible values are:

  • 'always' - Always highlight the tray icon
  • 'never' - Never highlight the tray icon, previously equivalent totray.setHighlightMode(false).
  • 'selection' - Highlight the icon when clicked and when the context menu is open (the default), previously equivalent to calling tray.setHighlightMode(true).

It still handles a boolean param so it should be backwards compatible with the previous behavior.

Before After
screen shot 2016-07-26 at 2 06 31 pm screen shot 2016-07-26 at 2 06 02 pm

Closes #6528

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jul 27, 2016

👍

@zcbenz zcbenz merged commit b8bafbc into master Jul 27, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3708037 succeeded in 45s
Details
electron-linux-ia32 Build #3708038 succeeded in 39s
Details
electron-linux-x64 Build #3708039 succeeded in 73s
Details
electron-mas-x64 Build #2017 succeeded in 6 min 15 sec
Details
electron-osx-x64 Build #2015 succeeded in 7 min 20 sec
Details
electron-win-ia32 Build #1028 succeeded in 6 min 27 sec
Details
electron-win-x64 Build #1015 succeeded in 6 min 30 sec
Details

@zcbenz zcbenz deleted the tray-highlight-toggle branch Jul 27, 2016

@davej

This comment has been minimized.

Contributor

davej commented Jul 27, 2016

@kevinsawicki: This is awesome!

My understanding is that to get selection working with windows (like in your screenshot), we would do something like this:

let win = new BrowserWindow({
  //...
})
tray.on('click', () => {
  (win.isVisible()) ? win.hide() : win.show()
})
win.on('show', () => {
  tray.setHighlightMode('always')
})
win.on('hide', () => {
  tray.setHighlightMode('never')
})

The semantics of always/never highlight mode seems a bit confusing in this case. Maybe it's worth clarifying in the docs because using a window seems to be quite a common case and people might think that they want selection mode at first glance.

davej added a commit to davej/menubar that referenced this pull request Jul 27, 2016

Highlight the menubar icon when menubar window is visible
Closes maxogden#103
This feature was added to electron in 1.3.1: electron/electron#6620
I've implemented it in a way that it doesn't break when used with earlier versions of electron.
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jul 27, 2016

The semantics of always/never highlight mode seems a bit confusing in this case.

Yeah, because Electron already had a highlight mode API, I went with expanding that API instead of creating a new one that would be used instead of setHighlightMode.

I don't think this was completely ideal, but I thought it was the best option to not break existing apps and having a second API would seem be to confusing as well, such as having both setHightlightMode and setHighlightIcon for example.

Your example looks correct though.

We could switch always/never to on/off but for apps that want to completely disable it, never felt like a good choice.

Maybe it's worth clarifying in the docs because using a window seems to be quite a common case and people might think that they want selection mode at first glance.

Yeah, sounds good, any ideas for what to add to the docs to make it clearer would be greatly appreciated.

@davej

This comment has been minimized.

Contributor

davej commented Jul 27, 2016

I don't think this was completely ideal, but I thought it was the best option to not break existing apps and having a second API would seem be to confusing as well, such as having both setHightlightMode and setHighlightIcon for example.

Yeah, I understand, that's fair.

We could switch always/never to on/off but for apps that want to completely disable it, never felt like a good choice.

Personally I prefer this but 1.3.1 is released so I suppose it's best to stick with always/never.

Yeah, sounds good, any ideas for what to add to the docs to make it clearer would be greatly appreciated.

I think having a code example of using tray with a window (like in my original comment) would clear up the ambiguities. Also perhaps emphasise that selection is for use with context menus. I can do a PR if you like.

One final thought, what do you think of coupling the tray with the associated window (just like context menus). You could then toggle the icon based on the visibility of win. Perhaps an API like this:

tray.setWindow(win)
tray.setHighlightMode('selection')

or alternatively:

tray.setHighlightMode('selection', win)
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jul 27, 2016

I can do a PR if you like.

That would be awesome, thanks 👍

One final thought, what do you think of coupling the tray with the associated window

That is an interesting idea that would definitely save some tray/window boilerplate event code that most apps have but I'm not sure initially whether it is better to have in Electron core or a userland library like menubar or something similar.

davej added a commit to davej/electron that referenced this pull request Jul 27, 2016

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