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

Added keyboard shortcut for displaying settings dialog. #1016

Conversation

MathewtheCoder
Copy link
Contributor

@MathewtheCoder MathewtheCoder commented Oct 17, 2019

Fixes

...

Checks

  • [ *] Ran npm run test-build

Changes proposed in this pull request:

@welcome
Copy link

welcome bot commented Oct 17, 2019

👋🏾Thanks for opening this pull request! Please check out our contributing guidelines.👊🏾😁

@imolorhe
Copy link
Collaborator

Hey, did you test the code? Looks like you have linting issues. Also shouldn't this be only in the electron app? This shortcut should open the Chrome or Firefox browser settings in the extensions and web apps right?

@imolorhe
Copy link
Collaborator

I just saw you also commented that you had similar doubts. So this should only work in the Mac electron app. For that, you'd need to update the menu.js in altair-electron with the preferences shortcut and also add an ipc event in the electron service

@imolorhe
Copy link
Collaborator

imolorhe commented Oct 17, 2019

So what I mean for the implementation is,

In the menu.js, add a new item somewhere around here:

https://github.com/imolorhe/altair/blob/staging/packages/altair-electron/src/menu.js#L53

for "Preferences..." and set an accelerator: 'Cmd+,'. For more info, on that you can look at the electron docs for MenuItem

The actions for the menu items are defined here (as you can see it just sends an event to the app):

https://github.com/imolorhe/altair/blob/staging/packages/altair-electron/src/window.js#L23-L42

You define the ipc channel listeners here:

https://github.com/imolorhe/altair/blob/staging/src/app/services/electron-app/electron-app.service.ts#L69-L71

GitHub
✨⚡️ A beautiful feature-rich GraphQL Client for all platforms. - imolorhe/altair
GitHub
✨⚡️ A beautiful feature-rich GraphQL Client for all platforms. - imolorhe/altair
GitHub
✨⚡️ A beautiful feature-rich GraphQL Client for all platforms. - imolorhe/altair

Copy link
Collaborator

@imolorhe imolorhe left a comment

Choose a reason for hiding this comment

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

You would need to implement this on the electron app as I have described. Hopefully my description was clear and easy enough for you 🙂

src/app/services/keybinder/keybinder.service.ts Outdated Show resolved Hide resolved
src/app/services/keybinder/keybinder.service.ts Outdated Show resolved Hide resolved
@imolorhe
Copy link
Collaborator

imolorhe commented Oct 17, 2019

@MathewtheCoder
Copy link
Contributor Author

Thanks for the descriptive comments. I will look into it tonight.

@MathewtheCoder
Copy link
Contributor Author

@imolorhe I have done the changes you had said and have removed the code for displaying the settings dialog on website. I was not able to run the electron app since I am quite new to electron app development. So please test the code and give feedback on how to improve the contribution.

@MathewtheCoder
Copy link
Contributor Author

@imolorhe How do we launch electron app. I tried building it but its throwing error at end ? Can you clarify on how to launch the electron app.

@imolorhe
Copy link
Collaborator

You need to run yarn test-all in the base of the repo, than go into the altair-electron package directory and run yarn dev

Copy link
Collaborator

@imolorhe imolorhe left a comment

Choose a reason for hiding this comment

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

Can you verify the changes before I merge them

@MathewtheCoder
Copy link
Contributor Author

@imolorhe Yes i will verify the changes and update in PR.

@MathewtheCoder
Copy link
Contributor Author

@imolorhe I have tested the electron app and it works as expected.
Menu Item
image
Settings Dialog
image

@imolorhe
Copy link
Collaborator

Nice! After the build passes, I'll merge. 🙂

@imolorhe imolorhe merged commit 98975a8 into altair-graphql:staging Oct 18, 2019
@welcome
Copy link

welcome bot commented Oct 18, 2019

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants