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

Revert Shortcut command for Clear Format on mac #2834

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

miku1958
Copy link
Contributor

@miku1958 miku1958 commented Oct 18, 2024

This shortcut is the same as the default global spotlight shortcut, so it is invalid if the user keeps spotlight‘s. Some users prefer to keep it the same as Windows by disable spotlight

@JiuqingSong
Copy link
Collaborator

@juliaroldi would you please comment here?
Thanks.

Copy link
Contributor

@juliaroldi juliaroldi left a comment

Choose a reason for hiding this comment

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

We need also check if the Control key is not pressed, otherwise the shortcut meta + control + space will not work to show the emoji panel

@miku1958
Copy link
Contributor Author

miku1958 commented Oct 29, 2024

We need also check if the Control key is not pressed, otherwise the shortcut meta + control + space will not work to show the emoji panel

@juliaroldi Sorry for late response, busy last week.

I don't think keystroke conflicts are a concern for this PR, and it's not just a problem with this shortcut, ShortcutPlugin should take care of additional keystroke triggers.

On the other hand, emoji panel is using the browser shortcut, which has highest priority in browser(like the spotlight in system).

@juliaroldi
Copy link
Contributor

We need also check if the Control key is not pressed, otherwise the shortcut meta + control + space will not work to show the emoji panel

@juliaroldi Sorry for late response, busy last week.

I don't think keystroke conflicts are a concern for this PR, and it's not just a problem with this shortcut, ShortcutPlugin should take care of additional keystroke triggers.

On the other hand, emoji panel is using the browser shortcut, which has highest priority in browser(like the spotlight in system).

Hi @miku1958, with the current implementation of the ShortcutPlugin, the meta + control + space will not trigger the emoji panel. Could please test this scenario on your branch? I did the test in Chrome Browser.

@miku1958
Copy link
Contributor Author

miku1958 commented Nov 5, 2024

@juliaroldi I do some tests and make some fix, please review it again thanks.

@JiuqingSong
Copy link
Collaborator

@miku1958 For some unknown reason the required check "license/cla" is not running. This happened before and it seems like a github issue. I don't have a way to manually trigger it. So to workaround, you can try to push some change again (say modify a comment, or any minor change), to trigger it.

Thanks.

@miku1958
Copy link
Contributor Author

miku1958 commented Nov 6, 2024

@JiuqingSong it works now

@JiuqingSong JiuqingSong enabled auto-merge (squash) November 6, 2024 17:41
@JiuqingSong JiuqingSong merged commit 53a7c76 into microsoft:master Nov 6, 2024
4 checks passed
BryanValverdeU added a commit that referenced this pull request Nov 15, 2024
* fix test

* test

* Merge text node and segments (#2846)

* Merge text segments

* Fix test

* merge node

* fix build and test

* add test

* Add test

* fix test

* Remove tablePreProcessor (#2849)

* Add change data and apiName to ContentChangedEvent when handle keyboard input (#2854)

* Fix #2857 (#2858)

* Revert Shortcut command for Clear Format on mac (#2834)

* revert Shortcut command for Clear Format on mac

* fix ctrl and meta can press in the same time

* update comment

* Allow browser's default paste behavior when pasting from Office Android (#2863)

* Implement shouldPreventDefaultPaste function

* fix build

* fix build

* list-bugs

* fix image floating

* Customize the behavior of merging format values (#2865)

* define types

* add callbacks

* use param instead

* fix test

---------

Co-authored-by: Jiuqing Song <[email protected]>

* updateVersion.json

* Fix merge segment issue (#2871)

---------

Co-authored-by: Julia Roldi (from Dev Box) <[email protected]>
Co-authored-by: Julia Roldi <[email protected]>
Co-authored-by: Jiuqing Song <[email protected]>
Co-authored-by: 庄黛淳华 <[email protected]>
Co-authored-by: Rain-Zheng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants