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

Optimization for Windows #28

Merged
merged 17 commits into from
Jan 7, 2018
Merged

Optimization for Windows #28

merged 17 commits into from
Jan 7, 2018

Conversation

superhooman
Copy link
Contributor

Added:

  • Window controls
  • Some dependencies and requirements (squirrel-windows, icon > 256x256)

Added:
- Window controls
- Some dependencies
Copy link
Owner

@bukinoshita bukinoshita left a comment

Choose a reason for hiding this comment

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

Great job with Windows Optimization @uenify! Can you make these changes? And also make sure to run $ yarn test or npm run test

main/index.js Outdated
height: 580,
resizable: false,
titleBarStyle: 'hiddenInset',
icon: join(__dirname, 'main/static/icon.icns')
Copy link
Owner

Choose a reason for hiding this comment

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

can you remove the switch statement and use

{
 frame: os.platform() !== 'win32',
  icon: os.platform() === 'win32' ? join(__dirname, 'main/static/icon.ico') : join(__dirname, 'main/static/icon.icns')
}

Also, resizable should true (at least for macOS)

'use strict'

const Page = ({ children }) => {
return (
<main>
{platform() === 'win32'
?
(<div className="win-bar">
Copy link
Owner

Choose a reason for hiding this comment

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

Can you create a separate component under components named win-controls.js so you can apply CSS only on this component instead having it global

eg:

// ./components/win-controls.js

'use strict'

const WinControls = () => {
  return (
    <div className="win-bar">
      <div className="win-controls">
        <div className="window_header_button -minimize" onClick={() => {
          remote.BrowserWindow.getFocusedWindow().minimize()
        }}/>
        <div className="window_header_button -close" onClick={()=>{
          remote.BrowserWindow.getFocusedWindow().close()
        }}/>
      </div>

      <style jsx>{`
        .win-bar{
          position: fixed;
          top: 0;
          width: 100vw;
          -webkit-app-region: drag;
          -webkit-app-region: drag;
        }

        .win-controls{
          float: right;
        }

        .window_header_button {
          width: 3pc;
          height: 30px;
          display: inline-block;
          background: no-repeat center center;
          background-size: 9pt 9pt;
          margin: 0;
          z-index: 10;
          -webkit-app-region: no-drag;
          transition: .2s background-color;
        }

        .window_header_button:not(.-close):hover {
          background-color: rgba(255, 255, 255, .1);
        }

        .window_header_button:not(.-close):active {
          background-color: rgba(255, 255, 255, .2);
        }

        .-minimize {
          background-image: url(data:image/svg+xml;base64,Cjxzdmcgd2lkdGg9IjEycHgiIGhlaWdodD0iMXB4IiB2aWV3Qm94PSIwIDAgMTIgMSIgdmVyc2lvbj0iMS4xIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIj4KICAgIDwhLS0gR2VuZXJhdG9yOiBTa2V0Y2ggNDAgKDMzNzYyKSAtIGh0dHA6Ly93d3cuYm9oZW1pYW5jb2RpbmcuY29tL3NrZXRjaCAtLT4KICAgIDx0aXRsZT5taW5pbWl6ZTwvdGl0bGU+CiAgICA8ZGVzYz5DcmVhdGVkIHdpdGggU2tldGNoLjwvZGVzYz4KICAgIDxkZWZzPjwvZGVmcz4KICAgIDxnIGlkPSJQYWdlLTEiIHN0cm9rZT0ibm9uZSIgc3Ryb2tlLXdpZHRoPSIxIiBmaWxsPSJub25lIiBmaWxsLXJ1bGU9ImV2ZW5vZGQiPgogICAgICAgIDxnIGlkPSJ3aW5kb3dfYnRuIiB0cmFuc2Zvcm09InRyYW5zbGF0ZSgtMzAuMDAwMDAwLCAtMjcuMDAwMDAwKSIgZmlsbD0iIzAwMDAwMCI+CiAgICAgICAgICAgIDxnIGlkPSJHcm91cCIgdHJhbnNmb3JtPSJ0cmFuc2xhdGUoMTIuMDAwMDAwLCAxMi4wMDAwMDApIj4KICAgICAgICAgICAgICAgIDxnIGlkPSJ0b3AtcmlnaHQtY29udHJvbHMiIHRyYW5zZm9ybT0idHJhbnNsYXRlKDE4LjAwMDAwMCwgMTAuMDAwMDAwKSI+CiAgICAgICAgICAgICAgICAgICAgPHJlY3QgaWQ9Im1pbmltaXplIiB4PSIwIiB5PSI1IiB3aWR0aD0iMTIiIGhlaWdodD0iMSIgZmlsbD0iI2ZmZmZmZiI+PC9yZWN0PgogICAgICAgICAgICAgICAgPC9nPgogICAgICAgICAgICA8L2c+CiAgICAgICAgPC9nPgogICAgPC9nPgo8L3N2Zz4=);
          background-size: 9pt 1px;
        }

        .-close {
          background-image: url(data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIxMiIgaGVpZ2h0PSIxMCIgdmlld0JveD0iMCAwIDEyIDEwIj4KICA8ZyBmaWxsPSJub25lIiBmaWxsLXJ1bGU9ImV2ZW5vZGQiIHN0cm9rZT0iI0ZGRkZGRiIgdHJhbnNmb3JtPSJ0cmFuc2xhdGUoMSkiPgogICAgPHBhdGggZD0iTTAsMCBMOS44OTgxNjU3NSw5LjkwMDgyMzk1IiBmaWxsPSIjZmZmZmZmIiAvPgogICAgPHBhdGggZD0iTTAuMTAxODM0MjUyLDAgTDEwLDkuOTAwODIzOTUiIHRyYW5zZm9ybT0ibWF0cml4KC0xIDAgMCAxIDEwLjEwMiAwKSIgZmlsbD0iI2ZmZmZmZiIgLz4KICA8L2c+Cjwvc3ZnPgo=);
        }

        .-close:hover {
          background-color: #eb0716;
          background-image: url(data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIxMiIgaGVpZ2h0PSIxMCIgdmlld0JveD0iMCAwIDEyIDEwIj4KICA8ZyBmaWxsPSJub25lIiBmaWxsLXJ1bGU9ImV2ZW5vZGQiIHN0cm9rZT0iI0ZGRkZGRiIgdHJhbnNmb3JtPSJ0cmFuc2xhdGUoMSkiPgogICAgPHBhdGggZD0iTTAsMCBMOS44OTgxNjU3NSw5LjkwMDgyMzk1IiBmaWxsPSIjZmZmZmZmIiAvPgogICAgPHBhdGggZD0iTTAuMTAxODM0MjUyLDAgTDEwLDkuOTAwODIzOTUiIHRyYW5zZm9ybT0ibWF0cml4KC0xIDAgMCAxIDEwLjEwMiAwKSIgZmlsbD0iI2ZmZmZmZiIgLz4KICA8L2c+Cjwvc3ZnPgo=);
        }

        .-close:active{
          background-color: #a1405c;
          background-image: url(data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIxMiIgaGVpZ2h0PSIxMCIgdmlld0JveD0iMCAwIDEyIDEwIj4KICA8ZyBmaWxsPSJub25lIiBmaWxsLXJ1bGU9ImV2ZW5vZGQiIHN0cm9rZT0iI0ZGRkZGRiIgdHJhbnNmb3JtPSJ0cmFuc2xhdGUoMSkiPgogICAgPHBhdGggZD0iTTAsMCBMOS44OTgxNjU3NSw5LjkwMDgyMzk1IiBmaWxsPSIjZmZmZmZmIiAvPgogICAgPHBhdGggZD0iTTAuMTAxODM0MjUyLDAgTDEwLDkuOTAwODIzOTUiIHRyYW5zZm9ybT0ibWF0cml4KC0xIDAgMCAxIDEwLjEwMiAwKSIgZmlsbD0iI2ZmZmZmZiIgLz4KICA8L2c+Cjwvc3ZnPgo=);
        }
      `}</style>
    </div>
  )
}

export default WinControls

Copy link
Owner

Choose a reason for hiding this comment

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

And on Page you just import this component and use it

{ platform() === 'win32' ? <WinControls/> : etc... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for code improvements! Now I'm a little ashamed for my "code style".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, what about

html{
  -webkit-app-region: drag
}

On windows it will block all click events

Copy link
Owner

Choose a reason for hiding this comment

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

Never be ashamed for you code style. We are always learning, I'm really thankful that you step up to help me with Windows 😃

Copy link
Owner

Choose a reason for hiding this comment

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

You can use the way you did

Does this work?

{platform() === 'win32' ? (
  <WinControls />
) : (
  <style global>{`
  html{
    -webkit-app-region: drag
  }
`}</style>
)}

Copy link
Owner

@bukinoshita bukinoshita left a comment

Choose a reason for hiding this comment

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

Hey @uenify can you rebase your branch to to update with the latest changes on my branch? Also can you run npm run test and fix the lint issues?

@superhooman
Copy link
Contributor Author

I wasn't able to fix lint issues, because there was problems with linebreak-style on windows (Expected linebreaks to be 'LF' but found 'CRLF')
So, I've switched to macOS to solve this problems and fixed lint errors. 🤷‍♂️

@bukinoshita
Copy link
Owner

Hey @uenify! Great job, it's looking great.

Just 3 things I noticed

  • remove package-lock.json and add it to the .gitignore
  • you need to change here to <style jsx global> otherwise it return a warning.
  • I also trie to run electron-builder --win and it returns me this error:
TypeError: _this.logBuilding is not a function
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts:30:10
    at Generator.next (<anonymous>)
From previous event:
    at SquirrelWindowsTarget.build (/Users/bukinoshita/github/taskr/node_modules/electron-builder-squirrel-windows/out/SquirrelWindowsTarget.js:83:11)
    at taskManager.addTask.default.map.it (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/platformPackager.ts:120:85)
From previous event:
    at WinPackager.packageInDistributableFormat (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/platformPackager.ts:120:41)
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/platformPackager.ts:116:10
    at Generator.next (<anonymous>)
    at runCallback (timers.js:785:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)
From previous event:
    at WinPackager.pack (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/out/platformPackager.js:195:11)
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/packager.ts:345:24
    at Generator.next (<anonymous>)
    at xfs.stat (/Users/bukinoshita/github/taskr/node_modules/fs-extra/lib/mkdirs/mkdirs.js:56:16)
    at /Users/bukinoshita/github/taskr/node_modules/graceful-fs/polyfills.js:287:18
    at FSReqWrap.oncomplete (fs.js:155:5)
From previous event:
    at Packager.doBuild (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/out/packager.js:407:11)
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/packager.ts:289:52
From previous event:
    at Packager._build (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/out/packager.js:351:11)
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/src/packager.ts:259:23
    at Generator.next (<anonymous>)
    at runCallback (timers.js:785:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)
From previous event:
    at Packager.build (/Users/bukinoshita/github/taskr/node_modules/electron-builder-lib/out/packager.js:311:11)
    at /Users/bukinoshita/github/taskr/node_modules/electron-builder/src/builder.ts:287:40
    at Generator.next (<anonymous>)
From previous event:
    at _build (/Users/bukinoshita/github/taskr/node_modules/electron-builder/out/builder.js:61:21)
    at build (/Users/bukinoshita/github/taskr/node_modules/electron-builder/src/builder.ts:257:10)
    at then (/Users/bukinoshita/github/taskr/node_modules/electron-builder/src/cli/cli.ts:49:4)
    at runCallback (timers.js:785:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)
From previous event:
    at Object.args [as handler] (/Users/bukinoshita/github/taskr/node_modules/electron-builder/src/cli/cli.ts:49:4)
    at Object.runCommand (/Users/bukinoshita/github/taskr/node_modules/yargs/lib/command.js:228:22)
    at Object.parseArgs [as _parseArgs] (/Users/bukinoshita/github/taskr/node_modules/yargs/yargs.js:1041:24)
    at Object.get [as argv] (/Users/bukinoshita/github/taskr/node_modules/yargs/yargs.js:957:21)
    at Object.<anonymous> (/Users/bukinoshita/github/taskr/node_modules/electron-builder/src/cli/cli.ts:43:15)
    at Module._compile (module.js:612:30)
    at Object.Module._extensions..js (module.js:623:10)
    at Module.load (module.js:531:32)
    at tryModuleLoad (module.js:494:12)
    at Function.Module._load (module.js:486:3)
    at Function.Module.runMain (module.js:653:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3

Do you know what it might be? Can you run this command to build the binaries?

@superhooman
Copy link
Contributor Author

Weird, but when I change <style global> to <style jsx global> it doesn't work

Added package-lock.json to gitignore
@bukinoshita
Copy link
Owner

Really? That's weird. It should work according to their docs

@bukinoshita
Copy link
Owner

Were you able to run electron-builder --win to build the binaries?

@superhooman
Copy link
Contributor Author

superhooman commented Jan 7, 2018

It builds perfectly on Windows, but I wasn't able to build it on macOS using squirrel as a target.
(It creates win-unpacked folder with binaries which doesn't work)
But it builds nsis package which also doesn't work.
Maybe you should use Windows to build for Windows? (According to this article 🤷‍♂️)

@bukinoshita
Copy link
Owner

Yes, I saw that. I was just making sure that building Windows binaries in macOS does not work. I'll try to build on Windows and test it so I can merge.

Thanks!

@bukinoshita
Copy link
Owner

@uenify tested on Windows. I was able to compile everything just fine. It create squirrel and win-upacked for me. Tried to open squirrel and it returns me an error that I believe the AutoUpdater causes it. And win-upacked just show a black window.

@superhooman
Copy link
Contributor Author

It's my pleasure!
That pull request (it's my first one) taught me a lot. Also, thank you for your feedback and advice!
And I've got one more question
Why taskr does not work when building as nsis or msi? Just black screen. It seems that page is not loaded or something like that.

@bukinoshita
Copy link
Owner

That's awesome! I'm really thankful that you're helping me with it.

I don't know why. It happens to me as well. I couldn't fix.

One last request, I tried to run on my Windows PC and it returned me an error. So for now, would be better to just remove auto updater for Windows.

On main/index.js, can you verify that the platform is not win32?

if (platform() !== 'win32') {
  autoUpdater()
}

@bukinoshita bukinoshita merged commit 32249b2 into bukinoshita:master Jan 7, 2018
@bukinoshita
Copy link
Owner

Awesome! Thanks a lot @uenify :)

@superhooman
Copy link
Contributor Author

Finally :D
Not at all! And thank you too!

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

Successfully merging this pull request may close these issues.

2 participants