Keyboard Shortcuts: Bind Cmd/Ctrl+S as save #2863
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2863 +/- ##
==========================================
- Coverage 34.15% 33.88% -0.28%
==========================================
Files 340 191 -149
Lines 9400 5704 -3696
Branches 1606 999 -607
==========================================
- Hits 3211 1933 -1278
+ Misses 5292 3191 -2101
+ Partials 897 580 -317
Continue to review full report at Codecov.
|
editor/modes/visual-editor/index.js
Outdated
| @@ -73,6 +81,11 @@ class VisualEditor extends Component { | |||
| event.preventDefault(); | |||
| } | |||
|
|
|||
| save( event ) { | |||
There was a problem hiding this comment.
They use a new pattern in Calypso to avoid binding in the constructor:
save = ( event ) => {
...
}I think this is also how the official React codemod migrates React.createClass to class component.
There was a problem hiding this comment.
The feature you're referring to (public class fields property initializers) is actually not part of the language specification, but rather a stage 2 proposal. We've tended to avoid the most language proposal features out of fear for churn, and the syntax will fail to build. With class fields specifically there's been some recent revisions to combine proposals for public and private fields and unified class features with decorators that gives me cause for concern (introducing uncertainty to their stability).
There was a problem hiding this comment.
That makes a lot of sense in the context of WordPress development. I totally support this choice 👍 Thanks for taking your time to explain the rationale behind it :)
| } } /> | ||
| ); | ||
|
|
||
| keyPress( 68, document ); |
There was a problem hiding this comment.
It took me a few seconds to connect d char with 68 :)
Can we hide 68 behind the constant?
| */ | ||
| import KeyboardShortcuts from '../'; | ||
|
|
||
| describe( 'KeyboardShortcuts', () => { |
There was a problem hiding this comment.
I like this tests a lot. It helped me better understand changes introduced 👍
| keyPress( 68, document ); | ||
|
|
||
| expect( spy ).toHaveBeenCalled(); | ||
| } ); |
There was a problem hiding this comment.
Does it mean that if non-global keypress is intercepted by a node, then we could have the following test pass:
it( 'should not capture local key events globally', () => {
const spy = jest.fn();
const attachNode = document.createElement( 'div' );
document.body.appendChild( attachNode );
const wrapper = mount(
<div>
<KeyboardShortcuts
shortcuts={ {
d: spy,
} } />
<textarea></textarea>
</div>,
{ attachTo: attachNode }
);
keyPress( 68, wrapper.find( 'textarea' ).getDOMNode() );
expect( spy ).not.toHaveBeenCalled();
} );
|
This is a nice usability improvement, but I think I'd want to wait until after we resolve editor content not being kept in sync before moving to merge this, since as you note it can be confusing that Save is not available until moving focus. |
|
Fair enough, it can wait until |
Issue tracked at #2758, with an in-progress effort at #2418 which includes throttling (I think we may want to avoid). There's some extra considerations noted in the issue, including "Undo" behaviors and whether we'd want to refactor shape of editable value before incurring change events so frequently. |
|
Thanks @aduth for the update. I'd be interested to help you with the cross-linked issue. |
aduth
force-pushed
the
add/keyboard-shortcuts-save
branch
from
1db2e49 to
155ac8f
Compare
aduth
added
[Feature] Writing Flow
|
This one has sat stale for a while, but now that #4955 is finally merged, I did a rebase to bring this back up to date. Feels nice with the immediate cc @youknowriad |
|
Yaaay we have always-synced |
|
Nice! |
Related: #2179
Related: #71
In Progress: While this works, it could lead to unexpected results when adding a new paragraph with text and using the keyboard shortcut, since until #2758 is implemented the value of an Editable field is not synced with the post state until the field is blurred.This pull request seeks to add support for saving the post via Ctrl/Cmd+S .
Open questions:
Currently, this uses the same behavior as autosave, which is to save a draft if the post is a draft. There is no autosave behavior yet for published posts (#1295, #1773), so Ctrl/Cmd+S will do nothing in these cases. We could bind Cmd+S to update the published post, but the implementation here follows that of the current post editor.
Testing instructions:
Verify that Ctrl/Cmd+S saves the current post if editing the current editor.