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

calc: allow to use shortcut ctrl+Enter in comments #10622

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

meven
Copy link
Contributor

@meven meven commented Nov 28, 2024

this.map.mention can be unset in particular in calc.

Amends #10383

Change-Id: I8dba5d36f20d6fe3bf0df9b9fa15a98b70b51eaa

  • Resolves: #
  • Target version: master

Summary

In calc > insert a comment > type some text > hit Ctrl+Enter

Expected:
The popup closes, the comment is saved.

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

this.map.mention can be unset in particular in calc.

Signed-off-by: Méven Car <meven.car@collabora.com>
Change-Id: I8dba5d36f20d6fe3bf0df9b9fa15a98b70b51eaa
@meven meven requested review from vmiklos and hfiguiere November 29, 2024 16:59
Copy link
Contributor

@vmiklos vmiklos left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

Nits:

  • it would be good to be more verbose in the commit message. Ideally you would describe what's the purpose of this 'map.mention' (that's the at-completion in Writer, correct?) in Writer. Also describe how this failed (I assume this is always undefined in Calc/Impress?). Finally describe how the new state is meant to keep the old use-case fixed, while fixing the new use-case.
  • Would it be possible to cover this fix with a cypress test?

@vmiklos vmiklos merged commit 78eb5bb into CollaboraOnline:master Dec 2, 2024
13 checks passed
@meven
Copy link
Contributor Author

meven commented Dec 2, 2024

it would be good to be more verbose in the commit message. Ideally you would describe what's the purpose of this 'map.mention' (that's the at-completion in Writer, correct?) in Writer. Also describe how this failed (I assume this is always undefined in Calc/Impress?). Finally describe how the new state is meant to keep the old use-case fixed, while fixing the new use-case.

Too late to complete now.

Would it be possible to cover this fix with a cypress test?

I think so. Especially easy if inserting comment is already tested.

@meven meven deleted the work/meven/comment-calc branch December 2, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants