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

Components: Extract Reusable Keyboard Shortcuts #3644

Merged
merged 1 commit into from Nov 24, 2017

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Nov 24, 2017

Extract a reusable Keyboard Shortcuts component to the editor/components folder.
Needed to be able to separate the edit-post and editor module

Expect some similar PRs today, I'm going to merge them as soon as the tests pass, they consist of moving some files around.

Testing instructions

  • Try muti-selecting blocks using ctrl+A

@youknowriad youknowriad self-assigned this Nov 24, 2017

@youknowriad youknowriad merged commit e51fbe3 into master Nov 24, 2017

2 checks passed

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

@youknowriad youknowriad deleted the update/extract-reusable-global-keyboard-shortcuts branch Nov 24, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 24, 2017

Codecov Report

Merging #3644 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3644      +/-   ##
==========================================
- Coverage    36.9%   36.74%   -0.17%     
==========================================
  Files         271      274       +3     
  Lines        6630     6659      +29     
  Branches     1201     1205       +4     
==========================================
  Hits         2447     2447              
- Misses       3531     3556      +25     
- Partials      652      656       +4
Impacted Files Coverage Δ
...mponents/editor-global-keyboard-shortcuts/index.js 0% <0%> (ø)
editor/edit-post/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/publish-dropdown/index.js 0% <0%> (ø)
...or/edit-post/header/publish-with-dropdown/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 1856f8c...487d5d4. Read the comment docs.

codecov bot commented Nov 24, 2017

Codecov Report

Merging #3644 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3644      +/-   ##
==========================================
- Coverage    36.9%   36.74%   -0.17%     
==========================================
  Files         271      274       +3     
  Lines        6630     6659      +29     
  Branches     1201     1205       +4     
==========================================
  Hits         2447     2447              
- Misses       3531     3556      +25     
- Partials      652      656       +4
Impacted Files Coverage Δ
...mponents/editor-global-keyboard-shortcuts/index.js 0% <0%> (ø)
editor/edit-post/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/index.js 0% <0%> (ø) ⬆️
editor/edit-post/header/publish-dropdown/index.js 0% <0%> (ø)
...or/edit-post/header/publish-with-dropdown/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 1856f8c...487d5d4. Read the comment docs.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Jan 11, 2018

Contributor

@youknowriad, two questions:

  1. Shouldn't EditorGlobalKeyboardShortcuts be VisualModeKeyboardShortcuts instead? We now also have EditorModeKeyboardShortcuts (#3755), which is actually global and should also be renamed to express that.

  2. Shoud EditorGlobalKeyboardShortcuts be in @wordpress/components? It feels pretty tied to the editor, and the basic piece KeyboardShortcuts is already available in @wordpress/components anyway.

I'm looking at this in the context of #4323, and thinking ahead that that feature should be built as an extension. The two *KeyboardShortcuts components—the global and the visual-mode-only—should be hookable, perhaps via a .extraProps or .extraShortcuts filter.

Contributor

mcsf commented Jan 11, 2018

@youknowriad, two questions:

  1. Shouldn't EditorGlobalKeyboardShortcuts be VisualModeKeyboardShortcuts instead? We now also have EditorModeKeyboardShortcuts (#3755), which is actually global and should also be renamed to express that.

  2. Shoud EditorGlobalKeyboardShortcuts be in @wordpress/components? It feels pretty tied to the editor, and the basic piece KeyboardShortcuts is already available in @wordpress/components anyway.

I'm looking at this in the context of #4323, and thinking ahead that that feature should be built as an extension. The two *KeyboardShortcuts components—the global and the visual-mode-only—should be hookable, perhaps via a .extraProps or .extraShortcuts filter.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 11, 2018

Contributor

Shouldn't EditorGlobalKeyboardShortcuts be VisualModeKeyboardShortcuts instead? We now also have EditorModeKeyboardShortcuts (#3755), which is actually global and should also be renamed to express that.

Make sense

Shoud EditorGlobalKeyboardShortcuts be in @wordpress/components? It feels pretty tied to the editor, and the basic piece KeyboardShortcuts is already available in @wordpress/components anyway.

I don't think so since @wordpress/components is not editor specific while EditorGlobalKeyboardShortcuts is (in countrary to KeyboardShortcuts).

Contributor

youknowriad commented Jan 11, 2018

Shouldn't EditorGlobalKeyboardShortcuts be VisualModeKeyboardShortcuts instead? We now also have EditorModeKeyboardShortcuts (#3755), which is actually global and should also be renamed to express that.

Make sense

Shoud EditorGlobalKeyboardShortcuts be in @wordpress/components? It feels pretty tied to the editor, and the basic piece KeyboardShortcuts is already available in @wordpress/components anyway.

I don't think so since @wordpress/components is not editor specific while EditorGlobalKeyboardShortcuts is (in countrary to KeyboardShortcuts).

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Jan 11, 2018

Contributor

I don't think so since @wordpress/components is not editor specific while EditorGlobalKeyboardShortcuts is (in countrary to KeyboardShortcuts).

Yep, we're on the same page, I was surprised to see it in components.

Contributor

mcsf commented Jan 11, 2018

I don't think so since @wordpress/components is not editor specific while EditorGlobalKeyboardShortcuts is (in countrary to KeyboardShortcuts).

Yep, we're on the same page, I was surprised to see it in components.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 11, 2018

Contributor

It's not in components, it's in editor/components

There's also one important difference between EditorGlobalKeyboardShortcuts and EditorModeKeyboardShortcuts which is. The later is EditorGlobalKeyboardShortcuts are generic keyboard shortcuts (could be reused to build alternative layouts like P2 editor) while EditorModeKeyboardShortcuts is specific to the layout of the Post Editor edit-post directory.

Contributor

youknowriad commented Jan 11, 2018

It's not in components, it's in editor/components

There's also one important difference between EditorGlobalKeyboardShortcuts and EditorModeKeyboardShortcuts which is. The later is EditorGlobalKeyboardShortcuts are generic keyboard shortcuts (could be reused to build alternative layouts like P2 editor) while EditorModeKeyboardShortcuts is specific to the layout of the Post Editor edit-post directory.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Jan 11, 2018

Contributor

It's not in components, it's in editor/components

Oh, yes, I misread from my :grep. 🙈

while EditorModeKeyboardShortcuts is specific to the layout of the Post Editor edit-post directory.

👍

Contributor

mcsf commented Jan 11, 2018

It's not in components, it's in editor/components

Oh, yes, I misread from my :grep. 🙈

while EditorModeKeyboardShortcuts is specific to the layout of the Post Editor edit-post directory.

👍

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