Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-8042: Accepts ⌘+enter to comment (et al) #861

Merged
merged 1 commit into from
Feb 23, 2018
Merged

MM-8042: Accepts ⌘+enter to comment (et al) #861

merged 1 commit into from
Feb 23, 2018

Conversation

mkraft
Copy link
Contributor

@mkraft mkraft commented Feb 22, 2018

Summary

Adds ability to post comments with ⌘+enter (the original ticket). Also adds support for using ⌘ key in all places that control is supported in key combinations.

Ticket Link

MM-8042

Checklist

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed

@mkraft mkraft added the 2: Dev Review Requires review by a core commiter label Feb 22, 2018
@mkraft mkraft changed the title MM-8042: Accepts command+enter to post comment. MM-8042: Accepts command+enter to comment. Feb 22, 2018
@jasonblais jasonblais added this to the v4.8.0 milestone Feb 22, 2018
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

I think we should use the utils.jsx cmdOrCtrlPressed function here.

…ed utility function. Adds command support to some missing places.
@mkraft
Copy link
Contributor Author

mkraft commented Feb 23, 2018

@jespino Great idea. I have refactored to use Utils.cmdOrCtrlPressed in all places in the app that were using e.ctrlKey which adds some ⌘ key support that was missing too.

@mkraft mkraft changed the title MM-8042: Accepts command+enter to comment. MM-8042: Accepts command+enter to comment (et al) Feb 23, 2018
@mkraft mkraft changed the title MM-8042: Accepts command+enter to comment (et al) MM-8042: Accepts ⌘+enter to comment. Feb 23, 2018
@jespino
Copy link
Member

jespino commented Feb 23, 2018

@jwilander Can you confirm that is the expected behavior? Ctrl == Meta in mac in all cases, or are there exceptions?

@mkraft mkraft changed the title MM-8042: Accepts ⌘+enter to comment. MM-8042: Accepts ⌘+enter to comment (et al) Feb 23, 2018
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM. But I asked Joram about the Ctrl == Meta assumption, because I'm not 5/5 about it.

@mkraft
Copy link
Contributor Author

mkraft commented Feb 23, 2018

@jespino I manually tested each instance that was changed. I see no reason to not accept ⌘ key in all cases where the key combo uses control on windows. As a macOS user I expect that behaviour.

@jespino jespino removed the request for review from jwilander February 23, 2018 14:31
@jespino
Copy link
Member

jespino commented Feb 23, 2018

You are right, actually, the shortcuts help modal, use Ctrl for windows and linux and Meta for mac in all documented cases. So, Is enough for me :)

@jespino jespino added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Feb 23, 2018
@jwilander
Copy link
Member

Yep, @mkraft is correct

@jespino jespino merged commit b3d3735 into master Feb 23, 2018
@jespino jespino deleted the MM-8042 branch February 23, 2018 15:16
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Feb 23, 2018
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Mar 7, 2018
mkraft pushed a commit that referenced this pull request Mar 9, 2018
…use shared utility function. Adds command support to some missing places. (#861)"

This reverts commit b3d3735.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
7 participants