Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add keystroke for fullscreen mode #18048

Merged

Conversation

l-lejman
Copy link
Contributor

@l-lejman l-lejman commented Mar 4, 2025

Suggested merge commit message (convention)

Internal: Add keystroke for fullscreen mode.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@l-lejman l-lejman requested a review from Dumluregn March 4, 2025 16:51
@l-lejman l-lejman force-pushed the epic/1235-fullscreen-mode-presence-list branch from 5c81c22 to 3a1dd7a Compare March 6, 2025 12:34
@l-lejman l-lejman force-pushed the epic/1235-fullscreen-mode-hotkey branch from 1858615 to fd526c8 Compare March 6, 2025 15:06
Copy link
Contributor

@Dumluregn Dumluregn left a comment

Choose a reason for hiding this comment

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

After using the keystroke, it seems like browser steal the focus and the second stroke doesn't toggle editor fullscreen but is handled by the browser. We should add an editor.editing.view.focus(); call either here to the keystroke handler, or directly in the command (I think it makes sense - but then remember to remove this call from button press in UI plugin).

this.editor.accessibility.addKeystrokeInfos( {
keystrokes: [
{
label: t( 'Fullscreen mode' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a convention to name these entries to sound like human-friendly commands, i.e. start with a verb:

image

So this should be more like "Toggle fullscreen mode".

const t = this.editor.locale.t;

// Set the Ctrl+Shift+F keystroke.
this.editor.keystrokes.set( 'Ctrl+Shift+F', FULLSCREEN );
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we use such convention in some places (e.g. for bold plugin) but it's only useful if such const can be used in multiple places. Here we can use fullscreen as a command name directly. Or if we want to have it as a const, then also use it in line 47 where we add the command to the editor.

@l-lejman l-lejman requested a review from Dumluregn March 10, 2025 15:07
Base automatically changed from epic/1235-fullscreen-mode-presence-list to ck/epic/1235-fullscreen-mode March 10, 2025 15:44
@Dumluregn Dumluregn force-pushed the epic/1235-fullscreen-mode-hotkey branch from e2e313b to 32b96a2 Compare March 10, 2025 16:01
@Dumluregn Dumluregn merged commit 3b619ad into ck/epic/1235-fullscreen-mode Mar 10, 2025
1 check was pending
@Dumluregn Dumluregn deleted the epic/1235-fullscreen-mode-hotkey branch March 10, 2025 16:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants