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

Add shortcut tooltips for main toolbar #6605

Merged
merged 6 commits into from May 6, 2018
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented May 5, 2018

Description

This branch adds some shortcuts to tooltips in the main toolbar:

  • Undo: PRIMARY + Z
  • Redo: PRIMARY + Y (or SHIFT + PRIMARY + Z)
  • Save: PRIMARY + S

Questions:

  • Which one should "Redo" display?
  • Is it okay for "Save" to display just the shortcut without text (design-wise)? How should shortcuts but displayed for icon buttons with text? Cc @jasmussen.

How has this been tested?

Hover over the above mentioned buttons in the main toolbar.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@ellatrix ellatrix requested review from tofumatt, youknowriad, jasmussen and aduth May 5, 2018
Copy link
Member

tofumatt left a comment

I think we'll need to tweak the redo shortcut for Mac. That'll at least mess with some Safari folks.

const showTooltip = !! tooltip ||
// Should show the tooltip...
const showTooltip = (
// if an explicit tooltip is passed or...

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

I find this style of commenting each conditional really useful, especially after working on a patch around https://github.com/WordPress/gutenberg/blob/master/editor/components/block-list/block.js#L425 👍🏻

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

A nitpick is moving the "if" above might make more sense, eg:

// Should show the tooltip if...
  const showTooltip = (
    // an explicit tooltip is passed or...

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 5, 2018

Author Member

So move all "if"s? "Or if..."?

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

I think moving the if to the top and ending each line with or... or and..., along with the indentation, would make sense.

But what you have already makes sense, I just think the wording could be tidied up a bit. If my suggestion is less clear than I could be wrong and should be ignored 😅

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

I'd say it could be:

// Should show the tooltip if...
const showTooltip = (
	// an explicit tooltip is passed or...
	tooltip ||
	// there's a shortcut or...
	shortcut ||
	(
		// there's a label and...
		label &&
		// the children are empty and...
		( ! children || ( isArray( children ) && ! children.length ) ) &&
		// the tooltip is not explicitely disabled.
		false !== tooltip
	)
);

(Includes the lack of !! prefixes.)

// Should show the tooltip...
const showTooltip = (
// if an explicit tooltip is passed or...
!! tooltip ||

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

Any reason to use !! tooltip || rather than just the regular test for truthiness with tooltip ||?

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 5, 2018

Author Member

No. I copied the previous code.

( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitely disabled.

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

"explicitely" should be "explicitly" 🙂

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 5, 2018

Author Member

Ugh yes, I just copied from the previous comment. :)

function EditorHistoryRedo( { hasRedo, redo } ) {
return (
<IconButton
icon="redo"
label={ __( 'Redo' ) }
shortcut={ primaryShortcut( 'Y' ) }

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

Shift+Command+Z is the convention for "Redo" on Mac, so I wonder if we need yet another conditional helper here 😧

screenshot 2018-05-05 15 07 14

Command+Y in Safari will actually change the current page to the History page, even when a textarea is focused:
screenshot 2018-05-05 15 09 46

So this probably needs to be Shift+Command+Y or Shift+Command+Z, at least on MacOS.

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 5, 2018

Author Member

Interesting, didn't know about the Safari command clash. Interestingly this depends on the application:

screen shot 2018-05-05 at 18 51 50

Does SHIFT + PRIMARY + Z also make sense on Windows and Linux?

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

Sublime is probably an outlier because it's cross-platform and I think cares less about OS-specific stuff? Browsers are definitely all Shift+Command+Z:

screenshot 2018-05-05 18 07 27

screenshot 2018-05-05 18 06 57

Makes sense Sublime does Primary+Y as that's the Windows/Linux default. I checked in Edge and Primary+Y is redo there. Firefox in Windows uses Primary+Shift+Z. It's sort of a mess. Maybe we need a helper that does UA sniffing for this one? 😭

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

Actually, I spoke too soon. Edge also supports Primary+Shift+Z for redo in Gutenberg. Primary+Y is the default but it seems Edge (and all other browsers) also work with Primary+Shift+Z. I'm just testing Linux now but it works for Mac+Win...

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 5, 2018

Member

Linux works with Primary+Shift+Z too, so let's just use that one everywhere.

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 6, 2018

Author Member

Okay. Wondering if we should still keep Primary+Y then as we are blocking the browser shortcuts on Mac.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

I added a bit to the scope of this PR... It would be good if "raw" shortcodes can be used for both the KeyboardShortcuts component and TinyMCE, so they are all registered consistently with the same user agent check.

I wanted to try to do this a bit more dynamically, but maybe it'd be just better to explicitly create all functions? Not sure.

Copy link
Member

tofumatt left a comment

This is so much nicer!

more-red-than-green

editor.shortcuts.add( 'access+d', '', () => this.changeFormats( { strikethrough: ! this.state.formats.strikethrough } ) );
editor.shortcuts.add( 'access+x', '', () => this.changeFormats( { code: ! this.state.formats.code } ) );
editor.shortcuts.add( rawShortcut.primary( 'k' ), '', () => this.changeFormats( { link: { isAdding: true } } ) );
editor.shortcuts.add( rawShortcut.access( 'a' ), '', () => this.changeFormats( { link: { isAdding: true } } ) );

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 6, 2018

Member

Yay, when I saw these I thought "this could be refactored". This is great 👍

value: secondaryKeyCode( 'm' ),
label: secondaryShortcut( 'M' ),
value: rawShortcut.secondary( 'm' ),
label: displayShortcut.secondary( 'm' ),

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 6, 2018

Member

Nice, the consistency here is better 👍

return `${ keyCombo }+${ character }`;
}
const modifiers = {
primary: ( _isMac ) => _isMac() ? [ COMMAND ] : [ CTRL ],

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 6, 2018

Member

This is much more immediately understandable than my old way.

export function secondaryShortcut( character, _isMacOS = isMacOS ) {
return keyboardShortcut( secondaryKeyCode( character.toUpperCase(), _isMacOS ), _isMacOS );
}
export const displayShortcut = mapValues( modifiers, ( modifier ) => {

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 6, 2018

Member

Ah, cool. Sorry I didn't think to use this, I'm not used to having lodash around.

} );
} );

describe( 'accessKeyCode', () => {

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 6, 2018

Member

Looks like there are only tests for the rawShortcuts are gone. I guess the logic is really basic now so it matters less that there are no tests. 🤷‍♂️

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 6, 2018

Author Member

Ah, good catch, forgot to add tests for rawShortcode...

@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented May 6, 2018

I wanted to try to do this a bit more dynamically, but maybe it'd be just better to explicitly create all functions? Not sure.

I think it's fine to just create all the functions this way. It's not much code and it's very easy to trace and figure out.

ellatrix added 2 commits May 6, 2018
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

Thanks for the review @tofumatt! Wasn't sure if you'd like it. :) Added some of the tests back that I forgot.

Copy link
Member

tofumatt left a comment

New tests and changes looks good too! 🚢

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

Hm, on last thing to figure out: shift+command+z is not actually working with Mousetrap, neither in this branch nor in master. The callback is never called... (mod+z: this.undoOrRedo)

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

Strange, here it works: https://codepen.io/iseulde/pen/BxwvBB

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

Can't figure this one out immediately. In Codepen it works. meta+z works in Gutenberg. shift+meta+z fails in Gutenberg. I can also need the menu item being activated (not the case with meta+z). 🤔

@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented May 6, 2018

In Gutenberg:

In windows Ctrl+Shift+Z is working in master for me on Edge.

On Mac Meta+Shift+Z is working in master for me on Firefox.

🤷‍♂️

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented May 6, 2018

Confirmed. Firefox and Safari work, it's just Chrome that is broken. Actually the Codepen is also broken there, I was just testing that in Safari instead of Chrome...

Let's address this separately as this is also broken in master.

@ellatrix ellatrix merged commit 5e940bf into master May 6, 2018
2 checks passed
2 checks passed
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +27.84% compared to c37a222
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ellatrix ellatrix deleted the add/main-toolbar-shortcuts branch May 6, 2018
return `${ keyCombo }+${ character }`;
}
const modifiers = {
primary: ( _isMac ) => _isMac() ? [ COMMAND ] : [ CTRL ],

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2018

Member

Why do these need to be run as functions? Will they ever change over the lifetime of the app?

Edit: And if the answer is for testing, it shouldn't really impact the implementation so much. Can be "good" for it to be made generic, though ideally we have some caching along the way in runtime use, so we're not running _window.navigator.platform.indexOf( 'Mac' ) !== -1; hundreds of times per page session.

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 7, 2018

Member

It's all for testing; previously this was run once and stored in a constant, but it was also entirely untested. It could probably be cached somehow as a perf-versus-readability tradeoff. 😄

It shouldn't change in regular usage though, so yeah it's worth doing.

[ SHIFT ]: isMac ? '⇧shift' : 'Shift',
};
const shortcut = [
...modifier( _isMac ).map( ( key ) => get( replacementKeyMap, key, key ) ),

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2018

Member

You defeated my new ESLint custom rule! (#6247)

Which is to say, I should make this rule more strict 😛

const showTooltip = (
// an explicit tooltip is passed or...
tooltip ||
// there's a shortcut or...

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2018

Member

I think I mentioned elsewhere, but conceptually shortcut has nothing to do with IconButton and I wish it wasn't included here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.