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

Fix incorrect unlink shortcut #9903

Merged
merged 1 commit into from Sep 14, 2018

Conversation

Projects
None yet
2 participants
@talldan
Contributor

talldan commented Sep 14, 2018

Description

I spotted that the shortcut displayed on the tooltip for unlinking is incorrect. It displays the tooltip for linking instead of unlinking

How has this been tested?

  • Manual testing - add a shortcut in various blocks that implement the RichText component (paragraph, table, list). Hover over the unlink icon, the shortcut should be displayed as ^⌥S on a Mac

Screenshots

Before
screen shot 2018-09-14 at 11 21 14 am

After
screen shot 2018-09-14 at 11 16 54 am

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan self-assigned this Sep 14, 2018

@talldan talldan added the [Type] Bug label Sep 14, 2018

@talldan talldan added this to the 4.0 milestone Sep 14, 2018

@talldan talldan requested a review from WordPress/gutenberg-core Sep 14, 2018

@@ -154,6 +154,7 @@ class FormatToolbar extends Component {
const isActive = isFormatActive || isAddingLink;
return {
...control,
shortcut: isFormatActive ? control.activeShortcut : control.shortcut,

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

Should we fallback too control.shortcut if control.activeShortcut is undefined? sometimes it's the same shortcut to activate/deactive a format right?

@youknowriad

youknowriad Sep 14, 2018

Contributor

Should we fallback too control.shortcut if control.activeShortcut is undefined? sometimes it's the same shortcut to activate/deactive a format right?

This comment has been minimized.

@talldan

talldan Sep 14, 2018

Contributor

This code is only specific to the link shortcut, so I haven't worried about a fallback:
https://github.com/WordPress/gutenberg/pull/9903/files#diff-cf43b17f636b0adcea4f5700de06c737R152

@talldan

talldan Sep 14, 2018

Contributor

This code is only specific to the link shortcut, so I haven't worried about a fallback:
https://github.com/WordPress/gutenberg/pull/9903/files#diff-cf43b17f636b0adcea4f5700de06c737R152

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

Oh got it :) It's not obvious at first sight in Github diff :P

The question is whether we should support this for other formats as block authors will be able to define alternative formats later (with the RichText tree structure work that's being done), it can be addressed later.

@youknowriad

youknowriad Sep 14, 2018

Contributor

Oh got it :) It's not obvious at first sight in Github diff :P

The question is whether we should support this for other formats as block authors will be able to define alternative formats later (with the RichText tree structure work that's being done), it can be addressed later.

This comment has been minimized.

@talldan

talldan Sep 14, 2018

Contributor

Hmm, yep, that might be a good idea - the same would need to apply for most of the other properties as well.

Thanks for the review!

@talldan

talldan Sep 14, 2018

Contributor

Hmm, yep, that might be a good idea - the same would need to apply for most of the other properties as well.

Thanks for the review!

@talldan talldan changed the title from Ensure correct shortcut for unlinking is displayed to Fix incorrect unlink shortcut Sep 14, 2018

@youknowriad

LGTM 👍

@youknowriad youknowriad modified the milestones: 4.0, 3.9 Sep 14, 2018

@talldan talldan merged commit ee6d092 into master Sep 14, 2018

1 check passed

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

@talldan talldan deleted the fix/incorrect-unlink-shortcut branch Sep 14, 2018

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