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

Keyboard shortcuts & command menu #705

Merged
merged 29 commits into from
Jan 11, 2025
Merged

Keyboard shortcuts & command menu #705

merged 29 commits into from
Jan 11, 2025

Conversation

hatemhosny
Copy link
Collaborator

@hatemhosny hatemhosny commented Jan 4, 2025

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert
  • 🌐 Internationalization / Translation

Description

This PR adds

  • Command menu (Ctrl + K)
  • Many keyboard shortcuts
  • The UI shows the keyboard shortcuts (in title attributes or <kbd> elements)
  • Keyboard shortcut menu showing the full collection of shortcuts

Related Tickets & Documents

as discussed in #630

Mobile & Desktop Screenshots/Recordings

image

image

image

image

image

image

image

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

TODO: add to features section in docs

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

[optional] Are there any post-merge tasks we need to perform?

translations

@zyf722 @gigamaster
Would you kindly have a look here at your convenience?
I'd like to know your general opinion, opinions about functionality, UI, selection of keyboard shortcuts and obviously if you find any issues.
Thank you.

Copy link

netlify bot commented Jan 4, 2025

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit 73cb1f4
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/678283b3679dd90008ec92bf
😎 Deploy Preview https://deploy-preview-705--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-workers-and-pages bot commented Jan 4, 2025

Deploying livecodes with Β Cloudflare Pages Β Cloudflare Pages

Latest commit: 73cb1f4
Status:Β βœ…Β  Deploy successful!
Preview URL: https://b640adc1.livecodes.pages.dev
Branch Preview URL: https://keyboard-shortcuts.livecodes.pages.dev

View logs

@zyf722
Copy link
Contributor

zyf722 commented Jan 8, 2025

A very useful feature! I believe this will be a great addition to the app πŸš€

Here are some issues and suggestions on it:

Potential Issues

  • Some actions like "About" and "Login" didn't work in some cases.
    • "Login" worked when I searched for "login" in the command palette, but not when no query was entered.
  • Scrollbar styles for these two screens are not applied in my Google Chrome 118. I tested with the latest version of Edge, and it worked fine there, so I guess there might be some compatibility issues.

scroll

Personal Suggestions

  • From my point of view, the "Keyboard Shortcuts" screen might be a bit confusing due to its similar appearance to the command palette. I would personally prefer using a table- or grid-based layout.

    • From Google Docs
      image
    • From Figma
      image
    • By the way, will users be able to customize the shortcuts in the future?
  • From an i18n perspective, I suggest using a bilingual approach for the command palette, where the English text is displayed alongside the localized text. This way, users can search for commands in both their preferred language and English.

    • Take a reference from what we have in VSCode:

@hatemhosny
Copy link
Collaborator Author

Thank you, @zyf722
These are all very useful comments

  • Some actions like "About" and "Login" didn't work in some cases.

    • "Login" worked when I searched for "login" in the command palette, but not when no query was entered.

I can reproduce that. I'll try to get a fix. Thank you.

  • Scrollbar styles for these two screens are not applied in my Google Chrome 118. I tested with the latest version of Edge, and it worked fine there, so I guess there might be some compatibility issues.

I did not get that in my browser, but I can think of a reason. I'll try to fix.

  • From my point of view, the "Keyboard Shortcuts" screen might be a bit confusing due to its similar appearance to the command palette. I would personally prefer using a table- or grid-based layout.

That's a good suggestion. It was just easy to implement, without code duplication by filtering the commands based if they have shortcuts and put it in a single list.
May be I can still get the data the same way but put that in a grid in a regular modal instead.
I'll explore and let you know.

  • By the way, will users be able to customize the shortcuts in the future?

I did not think of that.
Probably not for now. Possible things to think about:

  • We need to (locally) store the preferences as user data.
  • Avoid conflicts with other app shortcuts and editor shortcuts.
  • Some shortcuts do not work in the browser (e.g. Ctrl + N), even if you prevent default.

I think I will leave it like this for now and revisit this suggestion later.

  • From an i18n perspective, I suggest using a bilingual approach for the command palette, where the English text is displayed alongside the localized text. This way, users can search for commands in both their preferred language and English.

Good Idea!
Do we need to show both?
The library I use allows you to add a string of text to be used for search result without having to show it.

Thank you very much.
That was very helpful.

@hatemhosny
Copy link
Collaborator Author

@zyf722

  • Some actions like "About" and "Login" didn't work in some cases.

    • "Login" worked when I searched for "login" in the command palette, but not when no query was entered.

This was a race condition with double event firing. Fixed.

  • Scrollbar styles for these two screens are not applied in my Google Chrome 118. I tested with the latest version of Edge, and it worked fine there, so I guess there might be some compatibility issues.

This would be difficult to fix. The affected element is inside a web component with shadow DOM.
The styles are applied using ::part, which is only available in modern browsers.
Otherwise we will have to change the library source code.

  • From my point of view, the "Keyboard Shortcuts" screen might be a bit confusing due to its similar appearance to the command palette. I would personally prefer using a table- or grid-based layout.

How about this? :)

Screenshot (267)

image

  • From an i18n perspective, I suggest using a bilingual approach for the command palette, where the English text is displayed alongside the localized text. This way, users can search for commands in both their preferred language and English.

I added that. In languages other than English, English text will be displayed beneath each command. It can be searched.

Screenshot 2025-01-09 131600

Screenshot 2025-01-09 131833

Screenshot 2025-01-09 132112

Screenshot 2025-01-09 132132

Screenshot 2025-01-09 131939

I think this is much better. Thank you for the suggestions.

@zyf722
Copy link
Contributor

zyf722 commented Jan 10, 2025

@hatemhosny

Probably not for now. Possible things to think about:

  • We need to (locally) store the preferences as user data.
  • Avoid conflicts with other app shortcuts and editor shortcuts.
  • Some shortcuts do not work in the browser (e.g. Ctrl + N), even if you prevent default.

I agree with not implementing this feature for now. It indeed needs extra effort and consideration to implement this feature.

This would be difficult to fix. The affected element is inside a web component with shadow DOM.
The styles are applied using ::part, which is only available in modern browsers.
Otherwise we will have to change the library source code.

It's fully acceptable. Tried with BrowserStack and it does have something to do with the browser version. I believe we can leave it as is for now. Thank you for your efforts to fix this.

How about this? :)

Looks quite nice! Does the tip above change with other editors when selected by user, like CodeJar and CodeMirror?

I added that. In languages other than English, English text will be displayed beneath each command. It can be searched.

Great!

I think we could make it even more compact by removing the row gap and separator line between localized and English text. I believe the command palette will be more space-efficient this way.

@hatemhosny
Copy link
Collaborator Author

Looks quite nice! Does the tip above change with other editors when selected by user, like CodeJar and CodeMirror?

I use vscode key bindings extension for CodeMirror. So most of the shortcuts should also work.
Regarding CodeJar, I do not aim at feature pariry with other editors. It is stated in the docs that it is a minimal code editor that works only for trivial tasks.

I think we could make it even more compact by removing the row gap and separator line between localized and English text. I believe the command palette will be more space-efficient this way.

The top border and top padding are removed (also using ::part!)

image

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.3% Duplication on New Code (required ≀ 3%)

See analysis details on SonarQube Cloud

@hatemhosny hatemhosny merged commit 4338676 into develop Jan 11, 2025
17 of 18 checks passed
Copy link
Contributor

livecodes-ci bot commented Jan 11, 2025

i18n Actions

Source PR has been merged into the default branch.

Maintainers can comment .i18n-update-push to trigger the i18n update workflow and push the changes to Lokalise.

@hatemhosny hatemhosny deleted the keyboard-shortcuts branch January 11, 2025 20:52
@hatemhosny
Copy link
Collaborator Author

.i18n-update-push

Copy link
Contributor

livecodes-ci bot commented Jan 11, 2025

i18n Actions: .i18n-update-push

Localization updated and pushed to Lokalise.

Name Description
New Branch for i18n i18n/live-codes/keyboard-shortcuts
Last Commit SHA 4338676

Maintainers can comment .i18n-update-pull after translation is done to trigger the i18n pull workflow and pull the changes back to Github.

@zyf722
Copy link
Contributor

zyf722 commented Jan 17, 2025

It seems there might be another bug.

Some actions are not sorted as the first action in the list, even they match the user's query exactly:

@hatemhosny
Copy link
Collaborator Author

This is related to using fuzzy search. If we disable fuzzy search we get less results but better sorting.
I think I will do that.
Thank you.

image

image

@hatemhosny
Copy link
Collaborator Author

.i18n-update-pull

Copy link
Contributor

livecodes-ci bot commented Jan 22, 2025

i18n Actions: .i18n-update-pull

Failed to perform action due to following reason: Branch i18n/live-codes/keyboard-shortcuts does not exist.

Please check action logs for more details.

@hatemhosny
Copy link
Collaborator Author

.i18n-update-pull

Copy link
Contributor

livecodes-ci bot commented Jan 22, 2025

i18n Actions: .i18n-update-pull

Localization pulled from Lokalise.

Name Description
i18n Branch i18n/live-codes/keyboard-shortcuts
Last Commit SHA f90dbe9
i18n PR #721

@livecodes-ci livecodes-ci bot mentioned this pull request Jan 22, 2025
12 tasks
@hatemhosny
Copy link
Collaborator Author

.i18n-update-pull

@gigamaster
Copy link
Contributor

This looks great. I'll try it asap. Well done ^_^/

@gigamaster
Copy link
Contributor

Translation ES, FR, IT, JA, PT

⚠ Note
It seems the same keyboard shortcut is used for sharing and saving !?

TODO - Test if any < tag- > is missing or overriten that could break the layout!

@hatemhosny
Copy link
Collaborator Author

hatemhosny commented Jan 28, 2025

Welcome back @gigamaster :)
I hope you are doing very well and had good travel.

Translation ES, FR, IT, JA, PT

Thank you very much. That's great.

It seems the same keyboard shortcut is used for sharing and saving !?

  • Save (Ctrl + S)
  • Save as (Ctrl + Shift + S)
  • Share (Ctrl + Alt + S)

Could you notice any inconsistent behaviour?

image

TODO - Test if any < tag- > is missing or overriten that could break the layout!

It seems they are good. When I filter by "Inconsistent placeholders", I get no results.
Am I correct @zyf722 ?
Also should we expect the scheduled sync to start a PR for the new changes on Lokalise master branch?

@gigamaster
Copy link
Contributor

Obviously I'm still jetlagged and must have misread the +alt and +shift keyboard shortcuts
After a nap I'll review my proxy settings as the SSH connection seems slow, or maybe it's just me πŸ˜…
I'll then merge and try the latest version. All good. Hope you're doing well ^_^/

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.

3 participants