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

Try: Add toggle editor mode shortcut #3755

Merged
merged 23 commits into from Dec 25, 2017

Conversation

Projects
None yet
6 participants
@vladanost
Contributor

vladanost commented Dec 1, 2017

Description

I find switching often from Visual to Code editor mode tiring process so I wanted to add a shortcut for that.

All editor related keyboard shortcuts are grouped under EditorGlobalKeyboardShortcuts which is a great move but for this case it causes problems. EditorGlobalKeyboardShortcuts is rendered from VisualEditor block and if I add the shortcut there, once it switches to TextEditor, shortcuts are no longer available.

My temp approach is to add another component EditorModeKeyboardShortcuts and render it from Layout component.
For the moment it works but I'm wondering:

  1. is this shortcut really needed for everyone or I'm the only one that find it very useful when developing blocks?
  2. Is this the right way to include such a functionality?
  3. I chose the shortcut to be mod+shift+alt+m, maybe better solution can be found.
  4. The name of the component EditorModeKeyboardShortcuts is making a confusion?

All suggestions are welcome. Thanks.

How Has This Been Tested?

npm run test-unit
Manually tested in the browser.

Types of changes

New feature (non-breaking change which adds functionality)

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Dec 1, 2017

Codecov Report

Merging #3755 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3755      +/-   ##
==========================================
- Coverage   39.08%   38.97%   -0.12%     
==========================================
  Files         290      292       +2     
  Lines        6967     6987      +20     
  Branches     1273     1280       +7     
==========================================
  Hits         2723     2723              
- Misses       3570     3583      +13     
- Partials      674      681       +7
Impacted Files Coverage Δ
components/menu-items/menu-items-group.js 0% <ø> (ø) ⬆️
components/menu-items/menu-items-toggle.js 0% <ø> (ø) ⬆️
components/menu-items/menu-items-shortcut.js 0% <0%> (ø)
editor/edit-post/header/mode-switcher/index.js 0% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
editor/edit-post/modes/keyboard-shortcuts/index.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c97ebf8...915d1ec. Read the comment docs.

codecov bot commented Dec 1, 2017

Codecov Report

Merging #3755 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3755      +/-   ##
==========================================
- Coverage   39.08%   38.97%   -0.12%     
==========================================
  Files         290      292       +2     
  Lines        6967     6987      +20     
  Branches     1273     1280       +7     
==========================================
  Hits         2723     2723              
- Misses       3570     3583      +13     
- Partials      674      681       +7
Impacted Files Coverage Δ
components/menu-items/menu-items-group.js 0% <ø> (ø) ⬆️
components/menu-items/menu-items-toggle.js 0% <ø> (ø) ⬆️
components/menu-items/menu-items-shortcut.js 0% <0%> (ø)
editor/edit-post/header/mode-switcher/index.js 0% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
editor/edit-post/modes/keyboard-shortcuts/index.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c97ebf8...915d1ec. Read the comment docs.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 2, 2017

Contributor

Thanks for the PR, it's a good one, I can't speak for the chosen shortcut but it makes sense to me. Also, I recall mockups showing these shortcuts next to the button in the ellipsis menu for discoverability (cc @jasmussen).

Contributor

youknowriad commented Dec 2, 2017

Thanks for the PR, it's a good one, I can't speak for the chosen shortcut but it makes sense to me. Also, I recall mockups showing these shortcuts next to the button in the ellipsis menu for discoverability (cc @jasmussen).

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Dec 2, 2017

Contributor

@youknowriad Thanks. I moved the code to edit-post/modes/keyboard-shortcuts, will that work?
As for the shortcuts in the ellipsis menu I'm not sure where to put it since this shortcut toggles Visual/Code editor, it seems awkward to print it twice next to both names?

Contributor

vladanost commented Dec 2, 2017

@youknowriad Thanks. I moved the code to edit-post/modes/keyboard-shortcuts, will that work?
As for the shortcuts in the ellipsis menu I'm not sure where to put it since this shortcut toggles Visual/Code editor, it seems awkward to print it twice next to both names?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 18, 2017

Contributor

I'm not sure where to put it since this shortcut toggles Visual/Code editor, it seems awkward to print it twice next to both names?

I'd agree it wouldn't make much sense since tehre are 2 separate controls:

screen shot 2017-12-18 at 10 54 25

Hm the only way I can think of is merging the two controls in just one "dynamic" control:

  • in Visual mode the label would say "Switch to Code Editor"
  • in Code mode the label would say "Switch to Visual Editor"

However, I've heard some discussions about allowing plugins to add additional "modes" so I'm not sure the current UI would be so ideal in the first place.

Contributor

afercia commented Dec 18, 2017

I'm not sure where to put it since this shortcut toggles Visual/Code editor, it seems awkward to print it twice next to both names?

I'd agree it wouldn't make much sense since tehre are 2 separate controls:

screen shot 2017-12-18 at 10 54 25

Hm the only way I can think of is merging the two controls in just one "dynamic" control:

  • in Visual mode the label would say "Switch to Code Editor"
  • in Code mode the label would say "Switch to Visual Editor"

However, I've heard some discussions about allowing plugins to add additional "modes" so I'm not sure the current UI would be so ideal in the first place.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 18, 2017

Contributor

@afercia @vladanost @jasmussen We could show it next to the "Editor" label, can't we?

Contributor

youknowriad commented Dec 18, 2017

@afercia @vladanost @jasmussen We could show it next to the "Editor" label, can't we?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 18, 2017

Contributor

It's true that the editor mode switch was designed to be able to scale to more types, but probably this shouldn't keep us from making improvements right now.

It seems the feedback centers around having a single keyboard shortcut that would cycle between modes. So you'd invoke the shortcut to go to HTML mode, and the same shorcut would take you back.

Combined with #2980, we'd want to show that keyboard shortcut in the menu, right aligned.

Given the ellipsis menu for selecting the editing mode right now behaves like a radio group, it feels weird to show the same shortcut next to both items, even one that is active and unselectable. So can we just show the shortcut next to the item that's not selected? I.e. this:

✓ Visual Editor
  Code Editor   [⌘+Shift+Alt+M]

and subsequently ...

  Visual Editor [⌘+Shift+Alt+M]
✓ Code Editor

Would that work? It seems like the obvious thing to try.

The next step would be to make the item dynamic, like Andrea suggests.

Contributor

jasmussen commented Dec 18, 2017

It's true that the editor mode switch was designed to be able to scale to more types, but probably this shouldn't keep us from making improvements right now.

It seems the feedback centers around having a single keyboard shortcut that would cycle between modes. So you'd invoke the shortcut to go to HTML mode, and the same shorcut would take you back.

Combined with #2980, we'd want to show that keyboard shortcut in the menu, right aligned.

Given the ellipsis menu for selecting the editing mode right now behaves like a radio group, it feels weird to show the same shortcut next to both items, even one that is active and unselectable. So can we just show the shortcut next to the item that's not selected? I.e. this:

✓ Visual Editor
  Code Editor   [⌘+Shift+Alt+M]

and subsequently ...

  Visual Editor [⌘+Shift+Alt+M]
✓ Code Editor

Would that work? It seems like the obvious thing to try.

The next step would be to make the item dynamic, like Andrea suggests.

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Dec 22, 2017

Contributor

@youknowriad @jasmussen @afercia I tried to display the shortcuts by adding MenuItemsShortcut component.
Also added a separate map for shortcuts.
This is what I have at this stage:
mode-switcher

Contributor

vladanost commented Dec 22, 2017

@youknowriad @jasmussen @afercia I tried to display the shortcuts by adding MenuItemsShortcut component.
Also added a separate map for shortcuts.
This is what I have at this stage:
mode-switcher

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 22, 2017

Contributor

Really nice work. That looks good.

I'd remove the [] brackets, those were just for the ASCII mockup ;) — so long as the shortcut is right aligned. In the future we might need to either change the font size and/or dropdown width, to make room for long shortcuts. But nothing to worry about in this PR. Very nice work.

Contributor

jasmussen commented Dec 22, 2017

Really nice work. That looks good.

I'd remove the [] brackets, those were just for the ASCII mockup ;) — so long as the shortcut is right aligned. In the future we might need to either change the font size and/or dropdown width, to make room for long shortcuts. But nothing to worry about in this PR. Very nice work.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 22, 2017

Contributor

Really nice. A few questions/considerations:

  • will work on Windows/Linux and other OSes?
  • contrast is too low, especially against the gray background:

screen shot 2017-12-22 at 10 36 22

On a white background, the lightest gray that can be used is $dark-gray-300: #6c7781;. On the gray background, it should be a bit darker to have a contrast ratio of at least 4.5:1

  • can we avoid opacity? it makes calculating the actual contrast a bit hard. If necessary, I guess we can introduce a new gray in the color palette /cc @jasmussen

I chose the shortcut to be mod+shift+alt+m, maybe better solution can be found.

Yep, I think at some point all the shortcuts would need a final review to check for potential conflicts, I guess we can keep it as is for now!

Contributor

afercia commented Dec 22, 2017

Really nice. A few questions/considerations:

  • will work on Windows/Linux and other OSes?
  • contrast is too low, especially against the gray background:

screen shot 2017-12-22 at 10 36 22

On a white background, the lightest gray that can be used is $dark-gray-300: #6c7781;. On the gray background, it should be a bit darker to have a contrast ratio of at least 4.5:1

  • can we avoid opacity? it makes calculating the actual contrast a bit hard. If necessary, I guess we can introduce a new gray in the color palette /cc @jasmussen

I chose the shortcut to be mod+shift+alt+m, maybe better solution can be found.

Yep, I think at some point all the shortcuts would need a final review to check for potential conflicts, I guess we can keep it as is for now!

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 22, 2017

Contributor

Wondering if the + is really necessary? In macOS, the keys are separated with a small space. It's not just a visual thing: VoiceOver announces the plus and I guess other screen readers do the same (can't test on Windows right now). Removing it could help making things a bit more clear.

screen shot 2017-12-22 at 10 54 07

Contributor

afercia commented Dec 22, 2017

Wondering if the + is really necessary? In macOS, the keys are separated with a small space. It's not just a visual thing: VoiceOver announces the plus and I guess other screen readers do the same (can't test on Windows right now). Removing it could help making things a bit more clear.

screen shot 2017-12-22 at 10 54 07

export default {
toggleEditorMode: {
value: 'mod+shift+alt+m',
label: '⌘+Shift+Alt+M',

This comment has been minimized.

@youknowriad

youknowriad Dec 22, 2017

Contributor

Minor: I wonder if we should detect the platform and replace with ctrl if not on MacOS.

@youknowriad

youknowriad Dec 22, 2017

Contributor

Minor: I wonder if we should detect the platform and replace with ctrl if not on MacOS.

This comment has been minimized.

@vladanost

vladanost Dec 22, 2017

Contributor

Is it reliable to use navigator.platform? I can't find any example in the Gutenberg code.

@vladanost

vladanost Dec 22, 2017

Contributor

Is it reliable to use navigator.platform? I can't find any example in the Gutenberg code.

This comment has been minimized.

@youknowriad

youknowriad Dec 22, 2017

Contributor

I think it is but that's the first time we need it :)

@youknowriad

youknowriad Dec 22, 2017

Contributor

I think it is but that's the first time we need it :)

@youknowriad

LGTM 👍 Can you rebase your PR?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 22, 2017

Contributor

Also, I'm not able to merge can you rebase? git fetch origin && git rebase origin/master?

Contributor

youknowriad commented Dec 22, 2017

Also, I'm not able to merge can you rebase? git fetch origin && git rebase origin/master?

@youknowriad youknowriad merged commit e624d34 into WordPress:master Dec 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

getsource added a commit to getsource/gutenberg that referenced this pull request Jan 3, 2018

@@ -0,0 +1,54 @@
/**

This comment has been minimized.

@aduth

aduth Jan 16, 2018

Member

While I'd like for us to flatten and eliminate the modes directory altogether, I don't particularly like the creation of a new folder here, because as it was prior to these changes, the modes directory contains exclusively the supported editing modes for the editor: visual and text. The shortcuts are not in themselves a mode.

@aduth

aduth Jan 16, 2018

Member

While I'd like for us to flatten and eliminate the modes directory altogether, I don't particularly like the creation of a new folder here, because as it was prior to these changes, the modes directory contains exclusively the supported editing modes for the editor: visual and text. The shortcuts are not in themselves a mode.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jan 17, 2018

Member

Hi @vladanost @youknowriad

I was wondering if we could change this shortcut to fit in with the rest of WP and this editor. We have chosen Ctrl+Alt (Mac) and Shift+Alt (Windows) in the past because it can be entirely used by us without running into issues with browsers and operating systems. Also known as the access combination (we even got TinyMCE to move to that). Could we figure out a good combination for the mode switcher in this space? Any reason cmd/ctrl+shift+alt+m was chosen?

I think we should also build the API around that, so plugins can also only extend in this combination space.

Additionally, I can't figure out how to get the combination to work in master (Mac).

Member

iseulde commented Jan 17, 2018

Hi @vladanost @youknowriad

I was wondering if we could change this shortcut to fit in with the rest of WP and this editor. We have chosen Ctrl+Alt (Mac) and Shift+Alt (Windows) in the past because it can be entirely used by us without running into issues with browsers and operating systems. Also known as the access combination (we even got TinyMCE to move to that). Could we figure out a good combination for the mode switcher in this space? Any reason cmd/ctrl+shift+alt+m was chosen?

I think we should also build the API around that, so plugins can also only extend in this combination space.

Additionally, I can't figure out how to get the combination to work in master (Mac).

@iseulde iseulde referenced this pull request Jan 17, 2018

Closed

Add block-level keyboard shortcuts #4436

3 of 3 tasks complete
@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Jan 17, 2018

Contributor

Hi @iseulde
I'm currently on Linux and the shortcut combination works in master 2.0.
No particular reason for that combination, it should be revised.

Contributor

vladanost commented Jan 17, 2018

Hi @iseulde
I'm currently on Linux and the shortcut combination works in master 2.0.
No particular reason for that combination, it should be revised.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jan 17, 2018

Member

@vladanost It only seems to work if the pop up is open. If it's closed, it doesn't.

Member

iseulde commented Jan 17, 2018

@vladanost It only seems to work if the pop up is open. If it's closed, it doesn't.

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Jan 17, 2018

Contributor

@iseulde On Linux, it works regardless of the popup being open. I will make a test later today on Mac. (I remember it worked there too)

Contributor

vladanost commented Jan 17, 2018

@iseulde On Linux, it works regardless of the popup being open. I will make a test later today on Mac. (I remember it worked there too)

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Jan 17, 2018

Contributor

@iseulde

I think we should also build the API around that, so plugins can also only extend in this combination space.

Sound good.

Contributor

vladanost commented Jan 17, 2018

@iseulde

I think we should also build the API around that, so plugins can also only extend in this combination space.

Sound good.

@vladanost

This comment has been minimized.

Show comment
Hide comment
@vladanost

vladanost Jan 17, 2018

Contributor

@iseulde It works for me on Mac too.

Contributor

vladanost commented Jan 17, 2018

@iseulde It works for me on Mac too.

@iseulde iseulde referenced this pull request May 2, 2018

Merged

Add remaining editable shortcuts #4411

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment