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

IB-536 update to latest2 #13

Open
wants to merge 1,231 commits into
base: ckeditor-super-build
Choose a base branch
from

Conversation

VitalyT-Inspera
Copy link

@VitalyT-Inspera VitalyT-Inspera commented May 1, 2024

Too many changes are because latest master (CKEditor) is not merged to the ckeditor-super-build branch

pszczesniak and others added 30 commits March 26, 2024 12:48
Fix (ui): Users should be able to move the mouse cursor to a UI tooltip without closing it.
Fix (ui): Users should be able to close UI tooltips using the Esc key.
…or#15981)

Fix (watchdog): `EditorWatchdog` will no longer crash when the application is refreshed before completing the editor initialization or destruction. Closes ckeditor#15980.
…keditor#16103)

Internal (ui): Force fully unpin tooltip before pin due to memory leak issues in tests.
Other (core): Clarified the description of keystrokes that execute various buttons (Space, Enter) in the accessibility help dialog.
…ist-command

Internal (list): Introduced `customListNumbered` and `customListBulleted` commands for use in multi-level lists, with the ability to set additional attributes on command execution.
@VitalyT-Inspera VitalyT-Inspera self-assigned this May 1, 2024
@joaofolino
Copy link

5000+ files? I think this is a record or something.

window.editor = editor;
} )
.catch( error => {
console.error( 'There was a problem initializing the editor.', error );

Choose a reason for hiding this comment

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

What problem?

Choose a reason for hiding this comment

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

Can we make it like the one below, where there are no problems? :)
Please add some context to this error state. Why is it needed and why is this the best way to address it?

Copy link
Author

Choose a reason for hiding this comment

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

It is a sample used for verifying that build has performed correctly. It was provided by CKEditor team or this logic was moved from old superbuild branch. It is not used anywhere in our app and do not influence the ckeditor build

@@ -21,7 +21,7 @@ module.exports = {

output: {
// The name under which the editor will be exported.
library: 'ClassicEditor',
library: 'CKEDITOR',

Choose a reason for hiding this comment

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

The package name is for the classic editor. Maybe ClassicCKEditor?

Copy link
Author

Choose a reason for hiding this comment

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

This is how it was done in previous version (of our super build) and owr apps uses CKEDITOR name. I did it to maintain consistency. I agree that naming is not the most suitable. My suggestion is CKeditorInsperaBuild or InsperaCKEditor

@@ -1,5 +1,5 @@
{
"name": "@ckeditor/ckeditor5-build-classic",
"name": "ckeditor5-custom-build",

Choose a reason for hiding this comment

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

Why did you rename it to classic?

Copy link
Author

Choose a reason for hiding this comment

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

It is not me. It is done by whoever set up the ckeditor first. I just have to reintroduce this changes.
I am not happy this this approach either. How can reform CKEditor superbuild set up to the proper way? I mean forking CKEditor repo is overkill. The superbuild stuff should be done in another manner according CKEidtor docs - https://ckeditor.com/docs/ckeditor5/latest/installation/getting-started/quick-start-other.html#creating-custom-builds-with-online-builder

@joaofolino
Copy link

The error state needs some clearing up.

@VitalyT-Inspera
Copy link
Author

This changes can lead to grading build fail. We need to resolve this first - https://assessment.slack.com/archives/CELTV2XBL/p1714639668496849

@gmcatalin
Copy link
Collaborator

Only changes for upgrade ?

@gmcatalin
Copy link
Collaborator

Conflicts.

@VitalyT-Inspera
Copy link
Author

VitalyT-Inspera commented Jun 11, 2024

Conflicts.

resoved

Copy link
Collaborator

@gmcatalin gmcatalin left a comment

Choose a reason for hiding this comment

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

Bombs away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.