Skip to content

Conversation

@StormBurpee
Copy link
Contributor

Hi, thought I'd take a go at this.

Haven't got a windows machine near me to test it there, however you might want to look at making it a frameless window for windows. Then just add in custom close, max and minimise buttons. (Pretty easy, I can look at doing it for you if you like the look of this commit.)
Also, if you went down this route, we could look at hiding the minimise and maximise buttons when the sidebar is folded (I think that's what you guys call it.), so it just shows the close button, and doesn't overlap. Again this is all personal preference, but it would look nice.

Had to do a few style changes with the modal for preferences, other things where just personal preference to make it align and look how I thought it looked best. However you might want to make some small changes to some of the positioning of the elements. (I'd probably move the preferences button into the top bar)

Other than that, this should achieve what is brought up in issue #1055
The top bar is draggable and also changes with each style. :) Let me know what you guys think, or if you need me to change anything! It's pretty self explanatory however.

screen shot 2018-05-25 at 5 53 31 am

screen shot 2018-05-25 at 5 53 11 am

screen shot 2018-05-25 at 5 52 34 am

screen shot 2018-05-25 at 5 52 50 am

@ZeroX-DG
Copy link
Member

@StormBurpee Wow, it look nice! But if you switch to the frameless window, how do you access the old window menu on top?
image

@StormBurpee
Copy link
Contributor Author

Hi @ZeroX-DG I'll take a look at this now. If we want to go with the frameless window it might be a matter of creating a custom menu bar. Which I'm happy to look into right now. Other than that the other option is to just have this style on OSX, but for continuity it would look better if we had it on windows as well. The only thing is I don't have a windows machine for a few days to test on, so if someone could when I make the next commit that would be great!

@StormBurpee
Copy link
Contributor Author

Hey @ZeroX-DG and everyone else. I've made it so it works with windows now and has the menu bar. I was still testing this on a mac, hence why it has the extra menu options. But it will adapt to all the menus for the specified operating system. Here are a few images, let me know what you guys think.

So the mac version looks like this
screen shot 2018-05-25 at 2 16 29 pm

And now the windows version will look like this
screen shot 2018-05-25 at 4 56 03 pm
You might want to play with the svg icons for min/max/close and make them suit your style a bit better. However I did what I could to make it look neat.

On windows this also works with themes, so it follows along with the same style as mac! Just it's a solid colour this way.

Themes
screen shot 2018-05-25 at 4 56 58 pm
screen shot 2018-05-25 at 4 56 27 pm

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label May 25, 2018
Copy link
Contributor

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

please confirm my reviews!

and please fix from eslint!!

styleName='toolbar-controls'>
<ul className="menubar-list" styleName="menubar-list">
{Object.keys(this.state.menu).map(key => {
return <li className="menuItem" styleName="menu-item" onBlur={this.closemenu} onClick={() => this.openSubmenu(key)}>{this.state.menu[key].label}<ApplicationMenuDropdown menu={this.state.menu[key].submenu} open={this.state.openmenu == key ? true : false}></ApplicationMenuDropdown></li>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is so long! please indention!

styleName='toolbar-controls'>
<ul className="menubar-list" styleName="menubar-list">
{Object.keys(this.state.menu).map(key => {
return <li className="menuItem" styleName="menu-item" onBlur={this.closemenu} onClick={() => this.openSubmenu(key)}>{this.state.menu[key].label}<ApplicationMenuDropdown menu={this.state.menu[key].submenu} open={this.state.openmenu == key ? true : false}></ApplicationMenuDropdown></li>;
Copy link
Contributor

Choose a reason for hiding this comment

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

please write like below(and fix from eslint):

Object.keys(this.state.menu).map(key =>
  <li
    className="menuItem"
    styleName="menu-item"
    onBlur={this.closemenu}
    onClick={() => this.openSubmenu(key)}>
      {this.state.menu[key].label}
    <ApplicationMenuDropdown
      menu={this.state.menu[key].submenu}
      open={this.state.openmenu == key ? true : false} />
  </li>)

onMouseMove={(e) => this.handleMouseMove(e)}
onMouseUp={(e) => this.handleMouseUp(e)}
>
<div className={config.is_win ? 'windows-app-header' : 'mac-app-header'} styleName={config.is_win ? 'windows-app-header' : 'mac-app-header'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you writer both className and styleName?
I think className is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

The same applies to other components.

styleName={this.state.open == true ? "root" : "root-hidden"}>
<ul className="dropdown-items"
styleName="dropdown-items">
{Object.keys(this.state.menu).map(key => {
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove {} and return.

@sosukesuzuki
Copy link
Contributor

sosukesuzuki commented May 28, 2018

please use yarn instead of npm.

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 28, 2018
@StormBurpee
Copy link
Contributor Author

Hi, @sosukesuzuki
I've made all the changes requested in the review. Let me know if I need to do anything else.

Regards

@Rokt33r
Copy link
Member

Rokt33r commented Jun 5, 2018

@StormBurpee Please remove package-lock.json. It generates too many changes. And, could you fix the conflict?

@Rokt33r
Copy link
Member

Rokt33r commented Jun 5, 2018

Anyway, nice work! 😃 💯

Copy link
Contributor

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

please use yarn instead of npm.

please confirm my reviews:pray:

open: props.open
}

console.log(this.state.menu, 'submenu')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why console?
I want you to remove console.log.

this.state = {
isActive: false,
menu: props.menu.items,
open: props.open
Copy link
Contributor

Choose a reason for hiding this comment

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

I want you to don't use state in this case.
please use props.


return (
<div className='ApplicationMenuDropdown'
styleName={this.state.open === true ? 'root' : 'root-hidden'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

please write like below:

render  () {
  const { className, open, menu } = this.props
  const menuItems = menu.items
  return (
    ~~~~~~~
    styleName={open ? 'root' : 'root-hidden'}
    ~~~~~~~
    onClick={() => menuItems[key].click()}>
    ~~~~~~~
  )
}


this.toggleFullScreen = () => this.handleFullScreenButton()
const isWin = global.process.platform === 'win32'
ConfigManager.set({isWin: isWin})
Copy link
Contributor

Choose a reason for hiding this comment

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

please write like below:

ConfigManager.set({isWin})


this.toggleFullScreen = () => this.handleFullScreenButton()
const isWin = global.process.platform === 'win32'
ConfigManager.set({isWin: isWin})
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigManager.set({isWin})

@sosukesuzuki
Copy link
Contributor

@StormBurpee please confirm my review!

@kazup01
Copy link
Member

kazup01 commented Jul 17, 2018

How is the current state?

@Rokt33r
Copy link
Member

Rokt33r commented Jul 17, 2018

Close this pr now because it seems to be abandoned. If anyone is interested in this issue, please create another one.

@Rokt33r Rokt33r closed this Jul 17, 2018
@mikaoelitiana
Copy link
Contributor

I have created a new PR from this one and will fix issues from there #2318

mikaoelitiana added a commit to mikaoelitiana/Boostnote that referenced this pull request Aug 21, 2018
mikaoelitiana added a commit to mikaoelitiana/Boostnote that referenced this pull request Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants