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

API height menubar macos #9572

Merged
merged 7 commits into from Jun 5, 2017

Conversation

Projects
None yet
3 participants
@shubham2892
Contributor

shubham2892 commented May 24, 2017

Added an API to get MACOS X menubar height in the screen module using the [NSApp mainMenu] menuBarHeight API provided.
#8029

@@ -95,6 +95,12 @@ Returns [`Point`](structures/point.md)
The current absolute position of the mouse pointer.
### `screen.getMenuBarHeight()` _macOS_
Returns [`Integer`] - Height

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 24, 2017

Member

We don't need the "[" and "]" here 👍

This comment has been minimized.

@shubham2892

shubham2892 May 24, 2017

Contributor

Will fix this. 👍

This comment has been minimized.

@shubham2892

shubham2892 May 24, 2017

Contributor

Fixed :)

@shubham2892

This comment has been minimized.

Contributor

shubham2892 commented May 29, 2017

Hi @MarshallOfSound, Please let me know if this requires anymore changes. Thank you. 😄

@kevinsawicki

Left a few minor comments, thanks for adding this.

@@ -120,6 +120,9 @@ void Screen::BuildPrototype(
.SetMethod("getPrimaryDisplay", &Screen::GetPrimaryDisplay)
.SetMethod("getAllDisplays", &Screen::GetAllDisplays)
.SetMethod("getDisplayNearestPoint", &Screen::GetDisplayNearestPoint)
#if defined(OS_MACOSX)

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

This should be outdented to the beginning of the line.

@@ -120,6 +120,9 @@ void Screen::BuildPrototype(
.SetMethod("getPrimaryDisplay", &Screen::GetPrimaryDisplay)
.SetMethod("getAllDisplays", &Screen::GetAllDisplays)
.SetMethod("getDisplayNearestPoint", &Screen::GetDisplayNearestPoint)
#if defined(OS_MACOSX)
.SetMethod("getMenuBarHeight", &Screen::getMenuBarHeight)
#endif

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

This should be outdented to the beginning of the line.

namespace atom {
namespace api {
#if defined(OS_MACOSX)

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

This file will only be compiled on macOS so you don't need this #if here.

@@ -0,0 +1,15 @@
#import "atom/browser/api/atom_api_screen.h"

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

This file needs a license header, please copy it from another file.

namespace api {
#if defined(OS_MACOSX)
int Screen::getMenuBarHeight(){

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

Space needed between ) and {

@@ -135,6 +135,7 @@
'atom/browser/api/atom_api_protocol.h',
'atom/browser/api/atom_api_render_process_preferences.cc',
'atom/browser/api/atom_api_render_process_preferences.h',
'atom/browser/api/atom_api_screen_mac.mm',

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

This line should be added after 'atom/browser/api/atom_api_screen.h', to preserve the correct ordering.

@@ -120,6 +120,9 @@ void Screen::BuildPrototype(
.SetMethod("getPrimaryDisplay", &Screen::GetPrimaryDisplay)
.SetMethod("getAllDisplays", &Screen::GetAllDisplays)
.SetMethod("getDisplayNearestPoint", &Screen::GetDisplayNearestPoint)
#if defined(OS_MACOSX)
.SetMethod("getMenuBarHeight", &Screen::getMenuBarHeight)

This comment has been minimized.

@kevinsawicki

kevinsawicki May 30, 2017

Contributor

Please add a simple test for this method that verifies it returns a number to spec/api-screen-spec.js

@kevinsawicki kevinsawicki self-assigned this May 30, 2017

@shubham2892

This comment has been minimized.

Contributor

shubham2892 commented May 30, 2017

Hi @kevinsawicki, addressed the comments left by you. Thanks.

shubham2892 and others added some commits May 30, 2017

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 5, 2017

Thanks @shubham2892, great job on this 👍 🚀

@kevinsawicki kevinsawicki merged commit 9a362ee into electron:master Jun 5, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment