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

Remove menu bar on editor destroy. #16329

Merged

Conversation

mremiszewski
Copy link
Contributor

Suggested merge commit message (convention)

Tests (editor-multi-root): Properly remove the menu bar element on editor destroy.


Additional information

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

Copy link
Contributor

coderabbitai bot commented May 9, 2024

Walkthrough

The update to the CKEditor5's multirooteditor.js enhances the cleanup process in the destroyEditor() function. This improvement includes the removal of both the toolbar and menu bar elements when an editor instance is destroyed, ensuring a more thorough cleanup and preventing potential memory leaks or UI clutter from residual elements.

Changes

File Path Change Summary
.../ckeditor5-editor-multi-root/.../multirooteditor.js Enhanced destroyEditor() to remove menuBarView.element in addition to toolbar.element.

Recent Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 38a8f9c and 130c527.
Files selected for processing (1)
  • packages/ckeditor5-editor-multi-root/tests/manual/multirooteditor.js (1 hunks)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

@@ -44,6 +44,7 @@ function destroyEditor() {
editor.destroy()
.then( () => {
editor.ui.view.toolbar.element.remove();
editor.ui.view.menuBarView.element.remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing the order of operations in the destroyEditor function.

- editor.destroy()
-   .then( () => {
-     editor.ui.view.toolbar.element.remove();
-     editor.ui.view.menuBarView.element.remove();
-     window.editor = editor = null;
-     window.editables = null;
-     console.log( 'Editor was destroyed' );
-   } );
+ editor.ui.view.toolbar.element.remove();
+ editor.ui.view.menuBarView.element.remove();
+ editor.destroy()
+   .then( () => {
+     window.editor = editor = null;
+     window.editables = null;
+     console.log( 'Editor was destroyed' );
+   } );

The elements should be removed before calling editor.destroy() to ensure that all UI components are cleaned up before the editor instance is terminated. This helps prevent any potential issues that might arise if the editor's destruction process interferes with the removal of UI elements.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
editor.ui.view.menuBarView.element.remove();
editor.ui.view.toolbar.element.remove();
editor.ui.view.menuBarView.element.remove();
editor.destroy()
.then( () => {
window.editor = editor = null;
window.editables = null;
console.log( 'Editor was destroyed' );
} );

@DawidKossowski DawidKossowski merged commit 5bb1957 into release May 9, 2024
6 checks passed
@DawidKossowski DawidKossowski deleted the ck/remove-menu-bar-in-multiroot-menual-test branch May 9, 2024 10:56
earthtodani added a commit to AuthoriumDPA/ckeditor5-authorium that referenced this pull request May 30, 2024
* Docs: minor fixes. [short flow]

* Shorten the clipboard guide.

* Docs: formatting. [skip ci]

* Docs: timestamp bump. [skip ci]

* Docs: Unify the usage of quotation marks in the React guide.

* Docs: Update the Vue guide to new installation methods.

* Docs: Fix indentations in the React guide

* UI animations do not respect user's preferences on reduced motion (ckeditor#16099)

Feature: Editor UI should respect the user's preferences for reduced motion (WCAG 2.1, Success Criterion 2.3.3).

Feature (utils): Implemented the `env#isMotionReduced` flag to discover reduced motion preferences.

* Improved placeholders and font color grids in high contrast mode (ckeditor#16284)

Feature (utils): Implemented the `env#isMediaForcedColors` property for forced colors detection (e.g. high contrast mode on Windows). See ckeditor#14907.

Feature (ui): Implemented `ck-media-forced-colors` and `ck-media-default-colors` mixins for detecting forced colors (e.g. high contrast mode on Windows). See ckeditor#14907.

Fix (theme-lark): The caret should be visible in a placeholder while in forced colors mode (e.g. high contrast mode on Windows). Improved the look of the placeholders in the forced colors mode. Closes ckeditor#14907.

Fix (theme-lark): The color grid component should render as a grid of labels in the forced colors mode (e.g. high contrast mode on Windows). Closes ckeditor#14907.

* Docs: import link fix. [short flow]

* Use `dev-utils` with updated esbuild-loader

* Fix issues caused by targeting ES2022 (and dropping some polyfills)

* Image jumps when resizing in container that has padding (ckeditor#16304)

Fix (image, widget): An image should not jump upon resizing in a container with padding. Closes ckeditor#14698

---------

Co-authored-by: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

* Name, role, value accessibility fixes (ckeditor#16295)

Fix (restricted-editing): Improved the accessibility of the restricted editing dropdown by setting the correct ARIA role on the toolbar menu.

Fix (minimap): Addressed the issue of the minimap `<iframe>` being unnecessarily exposed to assistive technologies.   

---------

Co-authored-by: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

* Add menu bar integration to multi root editor. (ckeditor#16280)

Feature (editor-multi-root): Added menu bar support for multi-root editor.

* Other: Updated translations. [skip ci]

* Other: Updated translations. [skip ci]

* Internal (build-*): Builds.

* Add widget ESC shortuct accessibility dialog entry (ckeditor#16310)

Other (widget): Added information to the Accessibility help dialog about a keyboard shortcut that moves focus from a nested editable back to the widget.

---------

Co-authored-by: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

* Docs: Update the Next.js guide to new installation methods.

* Docs: Update builder links in integrations.

* No longer delay showing tooltip while focusing an element (ckeditor#16296)

Fix (ui): `TooltipManager` tooltips should immediately show up when triggered by user focus for better responsiveness and accessibility.

---------

Co-authored-by: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

* Mark mixin calls as pure

* Add `/* #__PURE__ */` before mixin calls in global scope

* Other: Updated translations. [skip ci]

* Internal (build-*): Builds.

* Docs: update Builder links. [short flow]

* Docs: update Builder links. [short flow]

* Use regular `dev-utils`

* Bump `ckeditor5-dev-*` packages to `^40.0.0`

* Use `declare` for `View.viewUid` property

* Updated the Accessibility guide with VPAT for the latest editor version and new keystrokes added to the list (ckeditor#16324)

Docs: Updated the Accessibility guide with VPAT for the latest editor version and new keystrokes added to the list.

* Docs: testing helpers fix. [short flow]

* Remove menu bar on editor destroy. (ckeditor#16329)

Tests (editor-multi-root): Removing the menu bar element on editor destroy.

* Docs: minor update. [skip ci]

* Docs: minor update. [skip ci]

* Add `declare` to few dynamically populated fields

* Docs: lists install update. [short flow]

* Docs: UI language link. [short flow]

* Docs: API links. [short flow]

* Docs: API links. [short flow]

* Docs: restoring new imports. [short flow]

* Update docs/tutorials/widgets/data-from-external-source.md

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>

* Update docs/tutorials/widgets/implementing-a-block-widget.md

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>

* Update docs/tutorials/widgets/implementing-a-block-widget.md

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>

* Update docs/tutorials/widgets/implementing-an-inline-widget.md

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>

* Docs: Updated the 2.5.7 section of the VPAT document.

* Internal: Move the internal `generatePositions` function to a static method on `BalloonPanelView` class.

* Docs: lotsa various fixes. [short flow]

* Docs: Fix link to `BalloonPanelView.generatePositions`

* Docs: lotsa various fixes. [short flow]

* Docs: lotsa various fixes. [short flow]

* Docs: Updated the 4.1.3 section of the VPAT document.

* Docs: expanding snippets. [short flow]

* Docs: crosslinking. [short flow]

* Increased specificity of image-style-align CSS classes.

* Internal: Vale evaluation. [skip ci]

* Docs: post-tests fixes. [short flow]

* Docs: Changelog. [skip ci]

* Docs: language fixes. [short flow]

* Docs: Corrected the changelog. [skip ci]

* Docs: Added an intro section. [skip ci]

* Docs: explaining Essentials. [short flow]

* Docs: minor rearrangements. [short flow]

* Increased specificity of almost all image-style-align CSS classes.

* Docs: updating and examples update. [short flow]

* Docs: await update. [short flow]

* Docs: setup rearrangements. [short flow]

* Docs: minor fix. [skip ci]

* Docs: Fix typos in the React guide.

* Docs: Update the Angular guide to new installation methods.

* Docs: review fixes and more changes. [short flow]

* Docs: removing the removing features section. [short flow]

* Docs: minor technical fix. [short flow]

* Docs: removing infoboxes. [short flow]

* Docs: removing redundant guides. [short flow]

* Docs: restoring the removal section. [short flow]

* Docs: restoring an internal link. [short flow]

* Changelog review.

* Docs: Prepare a structure for the Vue 2 guide.

* Docs: Unify subhedings in integrations.

* Docs: unsupported packages infobox.

* Docs: typos. [short flow]

* Docs: quick start update. [short flow]

* Docs: quick start update. [short flow]

* Docs: quick start update. [short flow]

* Docs: updating UI language guide. [short flow]

* Update ui on close image ckbox dialog.

* Docs: updste. [short flow]

* Docs: updste. [short flow]

* Docs: updste. [short flow]

* Internal: Improve tree-shaking by adding `#__PURE__` comments before function calls in global scope

* Update docs/getting-started/setup/ui-language.md

Co-authored-by: Filip Sobol <filipsobol@users.noreply.github.com>

* Docs: update. [short flow]

* Docs: updating Builder link. [short flow]

* Docs: updating API install commands. [short flow]

* Docs: updating API install commands. [short flow]

* Release: v41.4.0.

* Docs: Add the component installation step to the Vue 3 guide.

* Docs: Fix import in the Vue snippet.

* Docs: Fix formatting in the Vue guide.

* Docs: Add context to the Vue guide.

* Docs: Update snippets in the Vue 3 guide.

* Docs: Change the Angular guide structure.

* Docs: Unify collaboration sections across integrations.

* Docs: Fix typo.

* Docs: Add the localization section to the Vue 2 guide.

* Docs: Unify the localization section in Vue integrations.

* The TooltipManager should verify if the tooltip is not already unpinned while trying to update its position.

* Removed unnecessary non-null assertions.

* Updated the GH link to point to the public repo.

* Added info about the changes related to the silent release of NIM.

See: ckeditor#16360 (comment)

* Merge pull request ckeditor#16363 from ckeditor/cc/6219-update-race

Fix (ui): Prevented editor error in a situation where tooltip was unpinned after it was already removed. This happened when "Unlink" button was pressed while the tooltip was shown.

* Internal (build-*): Builds.

* Docs: Changelog. [skip ci]

* Docs: Changelog. [skip ci]

* Internal: An empty commit to trigger the release.

* Docs: Changelog.

* Release: v41.4.1.

* Docs: new methods READMEs.

* Merge master.

* Internal: Fixed wording in the `ImageResize` plugin description in `ckeditor5-metadata.json`. [skip ci]

* Docs: updating main README. [short flow]

* Docs: adding decoupled editor toolbar config. [short flow]

* Add clearTimeout and test.

* Docs: a typo. [skip ci]

* Update docs/getting-started/setup/toolbar.md

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>

* Docs: snippet update. [short flow]

* Docs: Update the Laravel integration guide to new installation methods.

* Added image style align left/right between image and image-inline.

* Fix (utils): Prevented error thrown when `ckeditor5-utils/src/env.ts` is used in an environment without `window` global object.

* Fixed lint issue.

* Rename `globalVar` to `global`.

* Docs (utils): Added a clarification remark to some properties in `env` object.

* Docs (utils): Grammar and typo fix.

* Internal (build-*): Builds.

* Docs: Changelog. [skip ci]

* Docs: Changelog (v41.4.2).

* Release: v41.4.2.

* Docs: removing builds. [short flow]

* Docs: Unify the setup sections.

* Docs: updates. [short flow]

* Docs: Prepare a structure for the .NET integration.

* Docs: Fix typo.

* Docs: Update the .NET integration guide to new installation methods.

* Docs: Fix contraction.

* Other: Add `#__PURE__` comments before objects and arrays in global scope.

* Internal: Vale evaluation. [skip ci]

* Docs: updating VPAT. [short flow]

* Instant pin tooltip on focus only if not hovered

* Docs: updating Drupal guide. [short flow]

* Docs: restoring a deleted file. [short flow]

* Add `onAfterDestroy` callback to react docs snippet, minor wording changes

* Docs: Minor fixes in the Quick Start

* Docs: Minor fixes in the Angular integration

* Docs: Minor fixes in the Next.js integration.

* Docs: Minor fixes in the Vue 2 integration.

* Docs: Minor fixes in the Vue integration.

* Docs: Modify the structure of the Next integration.

* Docs: Add new method to the React integration.

* Docs: minor fixes. [short flow]

* Core review fixes.

* Docs: puntuation. [short flow]

* Tweak plugins.

* Change spinner animation keyframe name to be more unique.

* Fix theme and CSS.

* Fix watchdog setup.

* Docs: general fixes. [skip ci]

* Docs: general fixes. [skip ci]

* Docs: relinking and cleanup. [short flow]

* Code review fixes.

* Docs: Unify the Builder naming.

* Code review fixes.

* Remove type.

* Docs: Remove excessive articles.

* Docs: Remove the excessive section from the Angular guide.

* Docs: Preliminary work in the Block widget guide.

* Docs: updating error codes. [short flow]

* Docs: updating error codes. [short flow]

* Docs: Update the Block widget tutorial to new installation methods.

* Docs: Fix typos in the Block widget guide.

* Docs: minor fixes. [short flow]

* Docs: pre-release fixes.

* Docs: integrations update. [short flow]

* Docs: adding CTA. [short flow]

* Docs: Preliminary work in the Inline widget tutorial.

* Docs: minor update. [short flow]

* Docs: minor update. [short flow]

* Docs: minor update. [short flow]

* Docs: Update the Inline widget tutorial to new installation methods.

* Docs: a review. [short flow]

* Docs: Builder infoboxes. [short flow]

---------

Co-authored-by: godai78 <b.biedrzycki@cksource.com>
Co-authored-by: Witek Socha <witek.soch@gmail.com>
Co-authored-by: Bartek Biedrzycki <68123541+godai78@users.noreply.github.com>
Co-authored-by: Mateusz Gorzeliński <mateusz.gorzelinski@gmail.com>
Co-authored-by: Mateusz Bagiński <cziken58@gmail.com>
Co-authored-by: Aleksander Nowodzinski <a.nowodzinski@cksource.com>
Co-authored-by: Filip Sobol <filipsobol@users.noreply.github.com>
Co-authored-by: Michał Remiszewski <118178939+mremiszewski@users.noreply.github.com>
Co-authored-by: Kamil Piechaczek <pomek@users.noreply.github.com>
Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>
Co-authored-by: CKEditorBot <ckeditor-bot@cksource.com>
Co-authored-by: Illia Sheremetov <i.sheremetov@cksource.com>
Co-authored-by: Kuba Niegowski <j.niegowski@cksource.com>
Co-authored-by: Szymon Cofalik <s.cofalik@cksource.com>
Co-authored-by: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-authored-by: Witek Socha <w.socha@cksource.com>
Co-authored-by: Paweł Smyrek <p.smyrek@cksource.com>
Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>
Co-authored-by: Daniela Soto <dsoto@eagerworks.com>
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.

2 participants