Skip to content
This repository has been archived by the owner on Nov 22, 2021. It is now read-only.

Add repost button to touchbar #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add repost button to touchbar #147

wants to merge 2 commits into from

Conversation

rmaes4
Copy link
Contributor

@rmaes4 rmaes4 commented Apr 26, 2017

Adds a repost button to the TouchBar. I also redid the icons, they now use the exact same assets as Soundcloud.

@rmaes4
Copy link
Contributor Author

rmaes4 commented Apr 26, 2017

Perhaps we should have media controls on the left, title in the center and the repost/like options on the right? Or some variation? What are your thoughts

Copy link
Owner

@salomvary salomvary left a comment

Choose a reason for hiding this comment

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

The new icons render much uglier than the old ones or the macOS built-in ones due to reduced resolution. Could you please restore the old icons or provide high resolution ones?

@@ -3,7 +3,7 @@
const { TouchBar } = require('electron')

const { TouchBarButton, TouchBarLabel, TouchBarSpacer } = TouchBar
const MAX_TITLE_LENGTH = 38
const MAX_TITLE_LENGTH = 39
Copy link
Owner

Choose a reason for hiding this comment

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

With one more button on the TouchBar I would expect the title length to become shorter not longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@salomvary I recreated the icons using the exact assets from Soundcloud. They are actually in SVG which you seemed to want earlier. Perhaps that would help.

I tested the title length and 39 is actually the maximum, the increase buttons seemed to have no effect. It seems although the 39 char length has more to do with the electron implementation (a bug possibly?)

@salomvary
Copy link
Owner

Perhaps we should have media controls on the left, title in the center and the repost/like options on the right? Or some variation? What are your thoughts

I don't have a strong opinion on button placement but I noticed the space available for the title has become quite tiny. Do we actually need all these buttons sacrificing a readable title?

@rmaes4
Copy link
Contributor Author

rmaes4 commented Apr 28, 2017

@salomvary I have opened an issue with electron that is currently being resolved in order to tighten up the buttons and free up space on the TouchBar. Personally I feel as though the buttons are worth it. The keyboard (and thus the TouchBar) is more for acting rather than looking (unless you look at the keys when you type). I guess my thought process is that buttons on the touch bar are more important than displaying the title, which would already be visible in the application.

@Impakt
Copy link

Impakt commented May 12, 2021

i would love to see this PR merged. Is there anything major stopping it? Also it seems as though the touch bar isn't working at all on my 2019 16" macbook pro

@salomvary
Copy link
Owner

@Impakt As this PR is quite old, it will take a bit of effort to make it work again and do some testing. Any help here would be appreciated.

@Impakt
Copy link

Impakt commented May 13, 2021

i actually pulled this repo and made the changes (the files that needed changing were very similar to how they were when the PR was created) on the master branch. When soundcleod has focus though the touch bar is empty. Getting the touch bar working again would be the hard part, i think adding repost would be easy.

I'm happy to test and help out as much as I can but i don't know electron or typescript that well (however been a java developer for over 20 years).

Edit: I even compared your touchbar code with the sample here in case some API had changed https://www.electronjs.org/docs/api/touch-bar

@Impakt
Copy link

Impakt commented May 13, 2021

I actually just got the touchbar working! i wrapped the array of touchbar controls at touchbar.js:48 with {items:[ .. array of touch bar controls .. ] }

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

Successfully merging this pull request may close these issues.

3 participants