Presence list and document outline for fullscreen mode #17975

l-lejman · 2025-02-24T08:38:14Z

Suggested merge commit message (convention)

Internal: Presence list and document outline for fullscreen mode.

Additional information

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

…plugin.

… into epic/1235-fullscreen-mode-presence-list

Dumluregn

I think it would make sense that left scroll should only be displayed to the height of document outline, not the whole sidebar.

With default styling, the H1 headings in document outline are larger than the "Document outline" caption, which seems odd - let's maybe discuss it on some meeting with Dagmara, I'll add it to the logbook.

There's an error in the console if you go to fullscreen, go back and click in the editable area:

Screen.Recording.2025-02-26.at.16.54.24.mov

Dumluregn · 2025-02-26T15:30:02Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

+		const presenceListWrapper = `
+			<div class="ck ck-fullscreen__left-sidebar-item">
+				<div class="ck ck-fullscreen__left-sidebar-header">Connected users</div>
+				<div class="ck ck-fullscreen__presence-list" data-ck-fullscreen="presence-list"></div>
+			</div>
+		`;
+
+		const fragment = document.createRange().createContextualFragment( presenceListWrapper );
+
+		document.querySelector( '[data-ck-fullscreen="left-sidebar-sticky"]' )!.appendChild( fragment );


Maybe for consistency across this one file we'll keep the convention of creating an element and setting its innerHTML here and later in DO?

Dumluregn · 2025-02-26T15:34:04Z

packages/ckeditor5-fullscreen/src/handlers/classiceditor.ts

+			this.generatePresenceListElement();
+			this.generateDocumentOutlineElement();


Let's try to keep in the specialized classes only the code that differs between them and the rest in the abstract editor, after calling the defaultEnable() and before executing the configured callback. It will make it easier to spot the differences (and perhaps unify the editor types one day to get rid of them).

Dumluregn · 2025-02-26T15:39:44Z

packages/ckeditor5-fullscreen/tests/handlers/abstracteditor.js

+import { PresenceListUI, PresenceList } from '@ckeditor/ckeditor5-real-time-collaboration';
+import { DocumentOutline, DocumentOutlineUI } from '@ckeditor/ckeditor5-document-outline';
+import mockCloudServices, { CloudServicesMock } from '@ckeditor/ckeditor5-real-time-collaboration/tests/_utils/mockcloudservices.js';


There's a problem with using the commercial features in the open source repository. We decided to use /* istanbul ignore next -- @preserve */ to prevent CC to fall below 100%, but provide the coverage on the premium packages side. So e.g. in document outline package there should be a test with fullscreen loaded that checks if everything is ok.

Dumluregn · 2025-02-26T15:41:19Z

packages/ckeditor5-fullscreen/tests/handlers/abstracteditor.js

+	let abstractHandler, domElement, editor, presenceList, documentOutline;
+
+	mockCloudServices();

 	beforeEach( async () => {
 		domElement = global.document.createElement( 'div' );
 		global.document.body.appendChild( domElement );
+		presenceList = document.createElement( 'div' );
+		document.body.appendChild( presenceList );
+		documentOutline = document.createElement( 'div' );
+		document.body.appendChild( documentOutline );

 		editor = await ClassicEditor.create( domElement, {
 			plugins: [
 				Paragraph,
-				Essentials
-			]
+				Essentials,
+				PresenceListUI,
+				PresenceList,
+				CloudServicesMock,
+				DocumentOutline,
+				DocumentOutlineUI
+			],
+			cloudServices: {
+				tokenUrl: 'abc',
+				webSocketUrl: 'web-socket-url'
+			},
+			collaboration: {
+				channelId: 'test'
+			},
+			presenceList: {
+				container: presenceList
+			},
+			documentOutline: {
+				container: documentOutline
+			}


For simplicity, I'd keep the config here minimalistic and create more complex editors on demand in tests that need them. But here as I wrote before we don't want to test the commercial features anyway.

Dumluregn · 2025-02-26T15:45:27Z

packages/ckeditor5-fullscreen/tests/manual/fullscreen.ts

@@ -52,6 +52,7 @@ import CloudServices from '@ckeditor/ckeditor5-cloud-services/src/cloudservices.
 import Style from '@ckeditor/ckeditor5-style/src/style.js';
 import GeneralHtmlSupport from '@ckeditor/ckeditor5-html-support/src/generalhtmlsupport.js';
 import Bookmark from '@ckeditor/ckeditor5-bookmark/src/bookmark.js';
+import PresenceList from '@ckeditor/ckeditor5-real-time-collaboration/src/presencelist.js';


This manual test throws for me right now as there is no .presence element in HTML, but let's only use open-source features in the manual test (I'm not sure if it even worked on CI) and premium features - using all-features-rtc manual test from commercial repo.

Dumluregn · 2025-02-26T15:47:20Z

packages/ckeditor5-fullscreen/theme/fullscreen.css

+
+.ck-fullscreen__left-sidebar {
+	font-family: Helvetica, Arial, sans-serif;
+	--ck-user-avatar-size: 32px;


Correct me if I'm wrong, but in presence list outside the fullscreen avatars have 40px and in annotations - 28px. I'm not sure if we really want to introduce a third size? In designs it's also 28px as far as I can see - https://www.figma.com/design/0fCM0UevmXtoL8DbVPt3wT/Full-screen?node-id=147-20312&m=dev.

in collapsed view it should be 32px because of white borders which looks like avatars are smaller than counter,
it was mentioned on some call and then i asked Dagmara if my changes looks fine
but i will remove 32px from not collapsed view

packages/ckeditor5-fullscreen/theme/fullscreen.css

Dumluregn · 2025-02-26T15:49:37Z

packages/ckeditor5-fullscreen/theme/fullscreen.css

 	margin-left: 10px;
-	margin-right: 10px;
 	background-color: hsl(34.29, 77.78%, 91.18%);


It's time to remove this color :D

Dumluregn · 2025-02-26T15:51:06Z

packages/ckeditor5-fullscreen/theme/fullscreen.css

 	display: flex;
 	justify-content: center;
 	overflow: auto;
 	max-height: 100%;
 }

-.ck-fullscreen__main-container .ck-fullscreen__sidebar {
+.ck-fullscreen__sidebar {
 	width: 270px;


According to the designs width should be 300px.

…hanges.

…outline.

…itor5 into epic/1235-fullscreen-mode-presence-list

Dumluregn · 2025-03-10T12:45:30Z

packages/ckeditor5-fullscreen/theme/fullscreen.css

In this file we have a snippet with fullscreen container layout, please update it according to the changes from the PR.

Dumluregn · 2025-03-10T12:46:50Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

@@ -196,6 +204,74 @@ export default class AbstractEditorHandler {
 		}
 	}

+	/* istanbul ignore next -- @preserve */
+	public generatePresenceListElement(): void {


Missing API docs for some of the new methods. They should also be private.

Dumluregn · 2025-03-10T12:47:38Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

+			return;
+		}
+
+		const presneceListElement = createElement( document, 'div', {


presneceListElement => presenceListElement

…ions.

…-mode-presence-list

l-lejman added 10 commits February 12, 2025 17:30

Add presence list integration to fullscreen mode.

da3d81d

Fullscreen mode - move sidebar to left side and add document outline …

095a7c1

…plugin.

Move scroll in left sidebar in fullscreen and change icons size.

9b60d5f

Fix scrolling issue in fullscreen.

b6f2e46

Merge branch ck/fullscreen-bootstrap of github.com:ckeditor/ckeditor5…

03530fd

… into epic/1235-fullscreen-mode-presence-list

Fix left sidebar in fullscreen template.

ce7d322

Add missisng deps in fullscreen.

78d5e0e

Merge branch ck/fullscreen-bootstrap of github.com:ckeditor/ckeditor5…

8b8891b

… into epic/1235-fullscreen-mode-presence-list

Add missing deps in fullscreen.

dde8ffe

Add tests for document outline and presence list.

93c3265

l-lejman requested a review from Dumluregn February 24, 2025 10:55

Fix disabling fullscreen mode.

88c0719

Dumluregn requested changes Feb 26, 2025

View reviewed changes

Left sidebar in fullscreen premium features tests removal and style c…

a27e885

…hanges.

Base automatically changed from ck/fullscreen-bootstrap to ck/epic/1235-fullscreen-mode March 5, 2025 09:53

Fix fullscreen mode on smaller screens and adjust styles of document …

3a1dd7a

…outline.

l-lejman force-pushed the epic/1235-fullscreen-mode-presence-list branch from 5c81c22 to 3a1dd7a Compare March 6, 2025 12:34

Merge branch ck/epic/1235-fullscreen-mode of github.com:ckeditor/cked…

6b58296

…itor5 into epic/1235-fullscreen-mode-presence-list

l-lejman requested a review from Dumluregn March 6, 2025 15:31

Fix document outline error after closing fullscreen mode.

c0498ce

Dumluregn requested changes Mar 10, 2025

View reviewed changes

l-lejman and others added 4 commits March 10, 2025 15:47

Left sidebar in fullscreen - fix typos and add comments for new funct…

e2b7bd6

…ions.

Add integration with iframe.

21b1f1c

Inject CKE stylesheet into iframe element in manual test.

c358b08

Merge branch 'ck/epic/1235-fullscreen-mode' into epic/1235-fullscreen…

cb6b1dd

…-mode-presence-list

Dumluregn approved these changes Mar 10, 2025

View reviewed changes

Dumluregn merged commit f0e38b2 into ck/epic/1235-fullscreen-mode Mar 10, 2025
3 of 7 checks passed

Dumluregn deleted the epic/1235-fullscreen-mode-presence-list branch March 10, 2025 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Presence list and document outline for fullscreen mode #17975

Presence list and document outline for fullscreen mode #17975

l-lejman commented Feb 24, 2025 •

edited

Loading

Dumluregn left a comment

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

l-lejman Mar 3, 2025

Dumluregn Feb 26, 2025

Dumluregn Feb 26, 2025

Dumluregn Mar 10, 2025

Dumluregn Mar 10, 2025

Dumluregn Mar 10, 2025

		this.generatePresenceListElement();
		this.generateDocumentOutlineElement();

Presence list and document outline for fullscreen mode #17975

Presence list and document outline for fullscreen mode #17975

Conversation

l-lejman commented Feb 24, 2025 • edited Loading

Suggested merge commit message (convention)

Additional information

Dumluregn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-lejman commented Feb 24, 2025 •

edited

Loading