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

feat: Add a key shortcuts modal #10879

Closed
reobin opened this issue Oct 16, 2020 · 10 comments
Closed

feat: Add a key shortcuts modal #10879

reobin opened this issue Oct 16, 2020 · 10 comments
Labels
type: refactoring code refactors

Comments

@reobin
Copy link
Contributor

reobin commented Oct 16, 2020

#10713 added a keyboard shortcut hook, and the migration for the current keyboard shortcuts to use the new hook has started. See #10878

Describe the solution you'd like

As suggested in #10713, the new hook could be used to populate a keyboard shortcut modal like GitHub's one:

Screen Shot 2020-10-16 at 08 54 53

The key ? would pop up that modal.

The details of the implementation have to be discussed. Using the hook to populate a keyboard shortcut store means that the modal would be different on a per-page basis depending on which shortcuts are used on the current page.

Also, they can't be added in there randomly. The shortcuts would need categorization, so the hook will probably need an update in order to receive the shortcuts categories.

Describe alternatives you've considered

It would be easier to just build a static modal, although less maintainable.

Additional context

#10468
#10713
#10878

@github-actions
Copy link
Contributor

Thanks for the issue! We'll take your request into consideration and follow up if we decide to tackle this issue.

To our amazing contributors: issues labeled type: bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem/oss and we will follow up within 3 business days.

For full info on how to contribute, please check out our contributors guide.

@citizen428
Copy link
Contributor

Thanks for the issue @reobin! Did you plan on implementing this yourself or is the issue up for grabs?

@reobin
Copy link
Contributor Author

reobin commented Oct 19, 2020

Yes, I'd like to work on this, thanks @citizen428 !

@Link2Twenty
Copy link
Contributor

I had a thought but I'm not sure how good an idea it is.

When we set up a shortcut we could 'register' the shortcut by putting it in an object on window (we could then remove it on unmount). We'd need to come up with an object model then we can read the variable in another component to show all, currently, registered shortcuts.

There is a chance this can go out of sync I guess but I'm not sure.

@citizen428
Copy link
Contributor

You probably want to involve @nickytonline in this discussion, but he's a bit swamped with Hacktoberfest work, so it may take a bit for him to reply.

@Link2Twenty
Copy link
Contributor

Link2Twenty commented Oct 20, 2020

I can flesh out my idea a little more now I'm at a keyboard.

useKeyboardShortcuts({
  Slash: () => { }
});

would become something like

useKeyboardShortcuts("Search", {
  Slash: {
    func: () => { }
    desc: "Set focus to the search bar"
  }
});

The useKeyboardShortcuts function would need some changes too

useKeyboardShortcuts(area, shortcuts, target) {

if (!window.shortcuts) window.shortcuts = {};

const windowTarget = window.shortcuts[area.toLowerCase()];
const keys = Object.keys(shortcuts);

if (!windowTarget) windowTarget = [];

for (let key of keys) {
  const keyExists = [...windowTarget].filter(obj => obj.key === key).length >= 1;
  if (keyExists) {
    console.alert(`useKeyboardShortcuts: ${key} for ${area} was registered more than once`);
    continue;
  }

  windowTarget.push({
    key: key,
    desc: shortcuts[key].desc
  })
}

and in window we'd see something like.

window.shortcuts.search = [{key: Slash, desc: "Set focus to the search bar"}]

window.shortcuts would then give us a list of areas for heading and the array within the heading would give us all the shortcuts.

@reobin
Copy link
Contributor Author

reobin commented Oct 20, 2020

No rush, the PRs mentioned still need to be merged.

In the mean time, let's work on using the keyboard shortcut hook on more stuff so that the modal has something to show :)

Sounds like a good idea @Link2Twenty! I don't think there is a solution implemented right now for a frontend global state, which could also be a good idea for this, but definitely needs discussion with the core team to go in the right direction.

@nickytonline
Copy link
Contributor

I'll hopefully get a peek at this before the end of the week.

@Link2Twenty
Copy link
Contributor

UI/UX: https://twitter.com/i/keyboard_shortcuts

@cmgorton
Copy link
Contributor

cmgorton commented Mar 1, 2021

Hey all! Thank you for opening this request.
Since this has been open for awhile without much progress I am going to close it here.
The Forem core team now uses an internal RFC ("request for comments") process to assess and prioritize new features. This process is intended to provide a consistent and standardized path for new changes to enter the Forem ecosystem. If you'd like to propose a new feature, please visit forem.dev to start a discussion around a new feature! 🙏

@cmgorton cmgorton closed this as completed Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring code refactors
Projects
None yet
Development

No branches or pull requests

7 participants