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

Make ScrubberItem width dynamic #11038

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
2 participants
@dashersw
Contributor

dashersw commented Nov 6, 2017

Depending on whether a ScrubberItem has text or an icon, this changeset calculates the actual width and sizes the TouchBar items accordingly. Previously, all ScrubberItems, regardless of their content, had a static width of 50px.

This commit also fixes #10539.

It simply turns this:

1

into this:

2

Make ScrubberItem width dynamic
Depending on whether a ScrubberItem has text or an icon, this changeset
calculates the actual width and sizes the TouchBar items accordingly.
Previously, all ScrubberItems, regardless of their content, had a static
width of 50px.

This commit also fixes #10539.

@dashersw dashersw requested a review from electron/reviewers as a code owner Nov 6, 2017

@welcome

This comment has been minimized.

welcome bot commented Nov 6, 2017

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@dashersw

This comment has been minimized.

Contributor

dashersw commented Nov 6, 2017

As a side note; I desperately need this feature to submit an app to the Mac App Store. I tried getting a custom build of Electron, but it turned out mission impossible. @paulcbetts told me if I could get a PR maybe this would end up in an official build soon so I can finally submit my app. Looking forward to your reviews.

Also — ObjectiveC is not my main language, so this may not be the best way to do it.

@MarshallOfSound

Looks legit 👍 Thanks for doing this 😄

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Nov 7, 2017

@dashersw What version of Electron are you using? Would you want this backported to 1.7.x?

@dashersw

This comment has been minimized.

Contributor

dashersw commented Nov 7, 2017

Thank you for the very fast response! @MarshallOfSound I use the latest beta, but when I submitted it to the App Store, the App Store reviewer told me it crashed on launch. I also had some problems as in #11037 where the code-signed release build crashed a few seconds later. I had to fix it by removing any native Network library (like Node's http, or even socket.io) and doing only XHR in the browser window.

I therefore would love to see it backported to 1.7.x, which hopefully doesn't have this crash either on the dev build or the MAS build — just to be safe. However, I didn't cherry-pick and make sure it works on 1.7.9. If you want I can also do that.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Nov 7, 2017

@dashersw I'll handle the back porting / testing 👍

@MarshallOfSound MarshallOfSound merged commit 43aa555 into electron:master Nov 7, 2017

5 of 6 checks passed

ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Nov 7, 2017

Congrats on merging your first pull request! 🎉🎉🎉

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