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

Display a shortcuts modal #3791

Merged
merged 14 commits into from
Jun 1, 2017
Merged

Display a shortcuts modal #3791

merged 14 commits into from
Jun 1, 2017

Conversation

ajithranka
Copy link

@ajithranka ajithranka commented Jan 25, 2017

Shows a subset of shortcuts from this wiki page on pressing ?.

screen shot 2017-01-25 at 12 48 19 pm

cc: @bhousel @kepta

@ajithranka ajithranka changed the title Display a shortcuts modal [WIP] Display a shortcuts modal Jan 25, 2017
@ajithranka ajithranka added the wip Work in progress label Jan 25, 2017
@ajithranka ajithranka changed the title [WIP] Display a shortcuts modal Display a shortcuts modal Jan 25, 2017
@bhousel
Copy link
Member

bhousel commented Jan 25, 2017

This is a great start! Next step is to move the English strings from shortcuts.json to core.yaml. Then shortcuts.json with contain translation keys and shortcuts.js will use t() to display them.

@ajithranka
Copy link
Author

@bhousel I couldn't make enough space in the modal (without making the width larger, allowing for a two column format) to show all shortcuts in the wiki. Is it ok to show only the most used shortcuts or should we display all of them?

@slhh
Copy link
Contributor

slhh commented Jan 25, 2017

@ajithranka We shouldn't generate secret shortcuts by omitting them from the display.

In case we want to omit something from the modal, we can omit all shortcuts for buttons which are directly visible on the UI, e.g. radial menu operations, or undo/redo/save. We can do that, because the operation is easily accessible by using the button, and the shortcut is discoverable due to the hover text.

In case we want to display all shortcuts the modal could have multiple tabs, or collapsible sections plus scrollbar.

@boothym
Copy link
Contributor

boothym commented Jan 26, 2017

I assume that the Mac command key will change to Ctrl when using a browser on Windows?

@bhousel
Copy link
Member

bhousel commented Jan 26, 2017

I assume that the Mac command key will change to control when using a browser on Windows?

Yes, iD.uiCmd (link) contains the code to adjust the keyboard shortcuts between Mac / Windows / Linux.

var description = p
.append('span')
.attr('class', 'col8')
.text(function(d) { return t('shortcuts.' });
Copy link
Collaborator

@kepta kepta Feb 8, 2017

Choose a reason for hiding this comment

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

@bhousel I want to form for e.g. t('shortcuts.display.zoom') over here. d would have d.key == 'zoom'. But what would be the best way to get display? and is this approach fine?

import { shortcuts as shortcutsData } from '../../data';

export function uiShortcuts() {
var key = uiCmd('⇧/');
Copy link
Member

Choose a reason for hiding this comment

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

I think we only need uiCmd when there's a ⌘ involved.

@kepta kepta removed the wip Work in progress label Feb 17, 2017
@kepta kepta self-assigned this Feb 17, 2017
@kepta
Copy link
Collaborator

kepta commented Feb 17, 2017

@bhousel lets merge this? 😎

@bhousel
Copy link
Member

bhousel commented Feb 20, 2017

@bhousel lets merge this? 😎

  • does it include all the keyboard shortcuts?

  • is shortcuts.json now the single source for the keyboard shortcuts across iD (good) or do we need to update shortcuts in multiple places (less good but not dealbreaker)
    yes this file is the single source for all the keyboard shortcuts.

  • export it as dataShortcuts, not shortcuts

  • does it work RTL?

  • does it work on smallish screens? is it responsive if I resize the browser window?

@kepta
Copy link
Collaborator

kepta commented Mar 9, 2017

rtl version
screen shot 2017-03-09 at 9 23 26 pm

small screen
screen shot 2017-03-09 at 9 17 21 pm

@kepta kepta force-pushed the shortcuts-modal branch from fb32fcf to fc577cc Compare March 9, 2017 15:57
@kepta
Copy link
Collaborator

kepta commented Mar 9, 2017

@bhousel made some changes.

We still need to figure out a clickable link which opens the shortcuts modal.
Currently it is by pressing ?.

@bhousel bhousel force-pushed the shortcuts-modal branch from fc577cc to 0e3b372 Compare May 16, 2017 19:36
@bhousel
Copy link
Member

bhousel commented May 16, 2017

Just rebased this branch to master. Looks like a great start - getting it merged will be my focus this week.

@runnabot
Copy link

runnabot commented May 31, 2017

Deployed iD.
View on Runnable

@bhousel bhousel merged commit 47664f3 into master Jun 1, 2017
@bhousel bhousel deleted the shortcuts-modal branch June 1, 2017 03:21
@magol
Copy link

magol commented Jun 2, 2017

How do I view this? When I press ? i only zoom in on the map (The + sign is on the same key on a Swedish keyboard)

@bhousel
Copy link
Member

bhousel commented Jun 2, 2017

Thanks @magol - I just made a change that should fix this, but I'm going to open a separate issue just in case it didn't.

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.

7 participants