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

Keymaps feature #1594

Merged
merged 18 commits into from May 22, 2024
Merged

Keymaps feature #1594

merged 18 commits into from May 22, 2024

Conversation

SuperDelphi
Copy link
Contributor

@SuperDelphi SuperDelphi commented May 6, 2024

Description

This pull request brings several pre-defined, ready-to-use shortcuts.

Shortcuts list

Full panels

  • Shift+Alt+S to open the Settings panel
  • Shift+Alt+P to open the Publish panel
  • Shift+Alt+F to open the Fonts panel
  • Tab to open/close the Preview mode

Left panels

  • Shift+L to open the Layers panel
  • Shift+A to open the Blocks panel ("a" for "add")
  • Shift+N to open the Notifications panel
  • Shift+P to open the Pages panel
  • Shift+S to open the Symbols panel
  • Escape to close the left panel

Workflow-specific

  • Shift+B to select the Body element

src/ts/client/grapesjs/index.ts Show resolved Hide resolved
src/ts/client/grapesjs/index.ts Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
@SuperDelphi
Copy link
Contributor Author

The branch has been updated!

@SuperDelphi SuperDelphi requested a review from lexoyo May 13, 2024 13:28
Copy link
Member

@lexoyo lexoyo left a comment

Choose a reason for hiding this comment

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

these things are still missing I believe for the feature to be complete

  • while in the text editor, esc should leave the text edition
  • the titles on the buttons should inform the user that there is a shortcut - cf screenshot bellow

Screenshot from 2024-05-14 10-17-30

my feedback about the choice of shortcuts, to be discussed

  • you where right, tab is a bad idea for preview, we do use tab for tab, sorry my bad
  • I would avoid shortcuts with 2 modifiers when possible? e.g "Full panels" could be alt+ instead of shift+alt+?
  • esc should close the last opened dialog - e.g. open publish, then esc, then close publish. Also right now it closes all pannels, e.g. i open the pages panel, then settings dialog, then esc => both are closed which is not what i intended (only settings should close)
  • esc in the settings when a text field is focused should close the settings, but instead it does nothing
  • good addition would be to pay attention to focus when a dialog opens (it is out of this feature I think, it should work also when open with a button), e.g. publish dialog focus the publish button

@SuperDelphi
Copy link
Contributor Author

SuperDelphi commented May 15, 2024

@lexoyo I added some functionality to my PR!

✅ There are shortcuts hints in the project bar buttons (including the Publish button).
✅ The Shift+Alt+... have been replaced with Alt+....
✅ Publish/Apply buttons are now automatically focused when the modal or the Publish dialog are opened, which can be triggered with Enter.
✅ The editor's input fields can be exited with the Escape key, except for (Rich) Text components (this is a complex issue).
✅ The Escape button is "smart" now. It will only close the foremost context. However, when exiting a "full panel" (ex: the Settings panel), even if you opened a "left panel" beforehand, the latter will be closed. It's an already existing issue due to the fact "left panels" close when a "full panel" opens. It is its own issue (that we can solve in another PR).

src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Show resolved Hide resolved
src/ts/client/grapesjs/keymaps.ts Outdated Show resolved Hide resolved
src/ts/client/grapesjs/settings.ts Outdated Show resolved Hide resolved
src/ts/client/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@SuperDelphi SuperDelphi left a comment

Choose a reason for hiding this comment

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

I've addressed the problems you raised:

  • Being able to select the body with Escape when there is no context opened.
  • Put back the autofocus of the first text input in a modal.

@lexoyo lexoyo merged commit fd17b5f into silexlabs:dev May 22, 2024
2 checks passed
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.

None yet

3 participants