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

Support quick access to top nav items with Alt 1–9 keyboard shortcuts #1302

Conversation

blabno
Copy link

@blabno blabno commented Jan 31, 2018

Fixes #1286

@blabno
Copy link
Author

blabno commented Jan 31, 2018

Supports left Alt on linux. I need to obtain Mac to test that rounted sharp key.

@cbeams
Copy link
Contributor

cbeams commented Jan 31, 2018

I just quickly tested on mac, none of the keys work.

@cbeams
Copy link
Contributor

cbeams commented Jan 31, 2018

Also, @blabno, it's probably best to implement this in a way that follows the existing approach for supporting keyboard shortcuts, which you can find at https://github.com/bisq-network/exchange/blob/master/gui/src/main/java/io/bisq/gui/app/BisqApp.java#L249-L284. If you follow that convention, these keyboard shortcuts should "just work" across all platforms, or at the very least, they will work as well as other, existing, keyboard shortcuts do across all platforms.

For example, line https://github.com/bisq-network/exchange/blob/master/gui/src/main/java/io/bisq/gui/app/BisqApp.java#L261, is where the CMD-J shortcut is configured, which opens the emergency wallet tool.

@blabno blabno force-pushed the feature/1286-support-quick-access-to-top-nav-items branch from fd7130c to e367df0 Compare January 31, 2018 09:51
@blabno
Copy link
Author

blabno commented Jan 31, 2018

@cbeams I've pushed new commit using suggested approach. You can give it a try.

@cbeams cbeams self-requested a review January 31, 2018 10:09
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

NACK. The substance of the PR looks good, and I've tested it locally on my Mac, where everything works as expected. Just need to clean up the imports and we're good.

import io.bisq.gui.common.view.FxmlView;
import io.bisq.gui.common.view.InitializableView;
import io.bisq.gui.common.view.View;
import io.bisq.gui.common.view.ViewLoader;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't expand these imports or the others that have been expanded below. The built-in IDEA settings should handle this correctly when running Optimize Imports. You may not be sync'd up correctly with those settings, or perhaps you're using some other IDE, in which case you'll need to take greater care to get it right manually.

In any case, please keep PRs atomic and avoid mixing in unrelated changes.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I didn't see .gitignore ignores .idea but actually keeps code style.
I've optimized imports using Bisq code style.

@blabno blabno force-pushed the feature/1286-support-quick-access-to-top-nav-items branch from e367df0 to 6453eaa Compare January 31, 2018 10:32
@cbeams
Copy link
Contributor

cbeams commented Jan 31, 2018

ACK, thanks @blabno!

Note that the next compensation request voting period starts tomorrow, and you have until end of day today to submit a request for this work if you'd like to. Otherwise you can always submit for a subsequent voting period, no problem.

Instructions are at https://github.com/bisq-network/docs/blob/master/dao/phase-zero.adoc#submit-a-compensation-request.

With regard to how much you should ask for, please gauge that based on how much value you believe this change adds to Bisq, the Bisq network, and its users. Not "how much time you spent on it". You can look at other compensation requests to get a gauge of things, and of course, feel free to ask around in Slack or elsewhere to see if the amount you're requesting seems reasonable to others.

@cbeams cbeams merged commit 6453eaa into bisq-network:master Jan 31, 2018
cbeams added a commit that referenced this pull request Jan 31, 2018
…s-to-top-nav-items

Support quick access to top nav items with Alt 1–9 keyboard shortcuts
@blabno blabno deleted the feature/1286-support-quick-access-to-top-nav-items branch January 31, 2018 10:53
@cbeams cbeams added this to the v0.6.5 milestone Jan 31, 2018
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