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 Keybinds window #178

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

Daniferrito
Copy link
Contributor

Implements #130

@Daniferrito
Copy link
Contributor Author

This is just a work in progress. Not sure if I can set that flag or not.

Here is how it looks so far (I changed the title of the window to "Keybinds" since the screenshot).

Screenshot_2019-09-25_21-26-11

Issues remaining:

  • Add a more unified way of adding this kind of window menus (maybe not needed if there are no more planned).
  • Sort the windows correctly. Right now, "Keybinds" is always on top of "DM Options". Not a big issue, but the most recent window could be moved to the top (by changing its z-index, i believe).
  • Add whatever keybinds are there that i did not add. I got this ones from the keyboard.ts file, but i'm not sure it is all of them
  • Allow the possiblity to edit the keybinds. Maybe not all of them (I would still keep the copy, paste and cut non-editable)
  • Fix the notes at the top of the movement section.

@Kruptein
Copy link
Owner

Already looks quite good!

In the future we might want to move some things to different tabs in that window, but for now it's more than fine.

I would indeed like to allow changing keybindings at one point, but this also needs server side capabilities to save the changes. and that can be added at a later instance.

Just have to check the code and play with it myself, but already seems merge-able

@Kruptein
Copy link
Owner

Missed the work in progress note, I can during PR creation click on a dropdown next to the 'create pull request' button to select 'create draft pr', unsure if that is also visible for you. Either way, just comment when it's finished

@Daniferrito
Copy link
Contributor Author

Oh, I created it from outside github, so i did not have that option. I will try it next time i create a PR (which should be soon, I have my eyes on an extra context-menu option to go to the nearest shape, to find the relevant part of the map again)

@Kruptein
Copy link
Owner

Related to that there is this which I still intend to add at some point. Basically a way to give quick-access to certain points. I'm unsure if that's something we want to add to the context menu, or rather have it in the sidebar menu like notes currently are.

@Daniferrito Daniferrito force-pushed the feature/keybinding-screen branch from 9959141 to 3fb9e32 Compare September 29, 2019 15:33
@Daniferrito Daniferrito force-pushed the feature/keybinding-screen branch from 3fb9e32 to ca9222d Compare September 29, 2019 15:45
@Daniferrito
Copy link
Contributor Author

Ok, more work done. I still dont like how the notes at the top of the movement section look, but I'm not sure how to change it, and The two other people that have seen it didn't see any problems.

A small bug that this introduces is that the Labels window is shown underneath the shape properties window that spawned it, as somehow the click event from Modal is triggered last no matter how I try to delay the moving to the top of the Labels window.

By the way, (as it is not in the main body of the PR), I just added a ModalsStore, that is in charge of reordering all the modals, by changing their z-index (was 9998 for all of them, it is now 8999 to 8000 in order of most-recent-interacted-with to least). I will probably change it so it also handles which modals are visible (it already knows, but currently visibility is handled by each individual modal).

Finally, I also extracted all the common logic of Closeable Modals into CloseableModal. It means that there are more EventBus subscriptions, as all Modals subscribe to its own Toggle, Open and Close, even if noone will ever trigger send them. I dont think this is a problem, though.

I will probably also add a key-bind for Escape that closes all modals, closes the top-left menu and clears the selection.

@Kruptein
Copy link
Owner

Haven't checked the code yet, but quickly ran it already. Some input:

  • I like the modal stacking change, seems to work pretty intuitive so far.
  • Escape closing all modals and menu's feels a bit weird. I would expect it to close the top-most modal/dialog and if no modal present perhaps the sidebar as well, although I would move that to another key (and have a dedicated key for the locations panel as well).
  • I don't like the current way the keybinds option is presented in the sidebar, I understand this is mostly due to my current styling rules, but this ideally looks cleaner in the final version
  • I would prefer to simply use 'keybindings' instead of keybinds in the sidebar as well as for the title of the keybinding dialog
  • The properties dialog in general is broken style-wise, it used to not be transparent at all, which it does now, hence some input fields looking strange.

Just as an aside from feedback from another DM (this does not have to be changed in this PR, but I would like to address it), I would drop the shift and only use ctrl for the multi selection. As shift is also commonly used to move assets through movement-blocking features, this can cause for accidental group movements when one just wants to move another shape and was already pressing shift.

@Kruptein
Copy link
Owner

Hey, just checking in to see if you still have time to look at this (if you are still using PA).
Otherwise I'll gladly continue work on this PR

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