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.

Sign up for GitHub

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

Jump to bottom

Add shortcut tooltips for main toolbar #6605

Merged
merged 6 commits into from
May 6, 2018
Merged

Add shortcut tooltips for main toolbar #6605

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5be0ff4
Add shortcut tooltips for main toolbar
ellatrix May 5, 2018
3fca8d0
Reorganise keycode file
ellatrix May 6, 2018
56389d6
Add rawShortcut tests
ellatrix May 6, 2018
a161ebc
Polish
ellatrix May 6, 2018
90c3e90
No longer needed to omit keyboardShortcut
ellatrix May 6, 2018
f042a70
Use rawShortcut everywhere
ellatrix May 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions components/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,21 @@ class IconButton extends Component {
const classes = classnames( 'components-icon-button', className );
const tooltipText = tooltip || label;

// Should show the tooltip if an explicit tooltip is passed
// or if there's a label and the children are empty and the tooltip is not explicitely disabled
const showTooltip = !! tooltip ||
// Should show the tooltip...
const showTooltip = (
// if an explicit tooltip is passed or...
Copy link
Member

Choose a reason for hiding this comment

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

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 👍🏻

Copy link
Member

@tofumatt tofumatt May 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

Choose a reason for hiding this comment

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

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.)

!! tooltip ||
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I copied the previous code.

// if there's a shortcut or...
!! shortcut ||
(
label &&
// if there's a label and...
!! label &&
// the children are empty and...
( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitely disabled.
Copy link
Member

Choose a reason for hiding this comment

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

"explicitely" should be "explicitly" 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

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

false !== tooltip
);
)
);

let element = (
<Button { ...additionalProps } aria-label={ label } className={ classes } focus={ focus }>
Expand Down
6 changes: 6 additions & 0 deletions editor/components/editor-history/redo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@ import { IconButton } from '@wordpress/components';
import { withSelect, withDispatch } from '@wordpress/data';
import { compose } from '@wordpress/element';

/**
* Internal dependencies
*/
import { primaryShortcut } from 'utils/keycodes';

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

@tofumatt tofumatt May 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@tofumatt tofumatt May 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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? 😭

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

disabled={ ! hasRedo }
onClick={ redo }
className="editor-history__undo"
Expand Down
6 changes: 6 additions & 0 deletions editor/components/editor-history/undo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@ import { IconButton } from '@wordpress/components';
import { withSelect, withDispatch } from '@wordpress/data';
import { compose } from '@wordpress/element';

/**
* Internal dependencies
*/
import { primaryShortcut } from 'utils/keycodes';

function EditorHistoryUndo( { hasUndo, undo } ) {
return (
<IconButton
icon="undo"
label={ __( 'Undo' ) }
shortcut={ primaryShortcut( 'Z' ) }
disabled={ ! hasUndo }
onClick={ undo }
className="editor-history__undo"
Expand Down
2 changes: 2 additions & 0 deletions editor/components/post-saved-state/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { withSelect, withDispatch } from '@wordpress/data';
*/
import './style.scss';
import PostSwitchToDraftButton from '../post-switch-to-draft-button';
import { primaryShortcut } from 'utils/keycodes';

/**
* Component showing whether the post is saved or not and displaying save links.
Expand Down Expand Up @@ -68,6 +69,7 @@ export class PostSavedState extends Component {
className="editor-post-save-draft"
onClick={ onSave }
icon="cloud-upload"
shortcut={ primaryShortcut( 'S' ) }
>
{ __( 'Save Draft' ) }
</IconButton>
Expand Down