feat(inline-tools): Inline tools rendered as popover #2718

TatianaFomina · 2024-05-11T14:43:08Z

New Inline Tools

Inline tools now use popover as well as Toolbox and Block Tunes 💪

What's done

PopoverInline class added. It extends PopoverDesktop but makes it horizontal (and adds few features)
Popover styles are updated
Removed redundant styles

ToDo

Tests
Rewrite built-in inline tools to new api

neSpecc

link icon is bigger that others
in header block, Link tool goes after Marker. Why?
"Conver to" tooltip is missed
Current state of buttons is highlighted differently

Bold tool is not working in Safari. Nothing happened on clicks.
All icons have different size on Mobile version

gohabereg · 2024-06-08T13:43:54Z

src/components/utils/popover/popover-abstract.ts

+    /**
+     * Handle 'mousedown' instead of 'click' event to prevent unwanted focus loss in safari
+     */
+    this.listeners.on(this.nodes.popoverContainer, 'mousedown', (event: Event) => this.handleClick(event));


Would it work on touch devices?

checked, works

gohabereg · 2024-06-08T13:48:31Z

src/components/utils/popover/popover-inline.ts

+    const totalLeftOffset = this.offsetLeft + itemOffsetLeft;
+
+    nestedPopoverEl.style.setProperty(
+      '--trigger-item-left',


I wonder if it would be better to put all the properties into some constants or enum?

added enum with variables

TatianaFomina · 2024-06-08T14:44:22Z

@neSpecc

in header block, Link tool goes after Marker. Why?

it's hardcoded in index.html

Changed

src/components/modules/api/selection.ts

src/components/modules/api/tools.ts

src/components/utils/blocks.ts

This reverts commit 8700f84.

This reverts commit 834391c.

neSpecc

I can't change a link url. Input is not focusable on clicks.

neSpecc · 2024-06-09T20:59:55Z

src/components/modules/dragNDrop.ts

@@ -69,7 +69,6 @@ export default class DragNDrop extends Module {
  private async processDrop(dropEvent: DragEvent): Promise<void> {
    const {
      BlockManager,
-      Caret,


Caret is used below. Probably, you should import the new caret position enum

neSpecc · 2024-06-09T21:01:32Z

src/components/modules/toolbar/conversion.ts

+import Block from '../../block';
+import { isEmpty } from '../../utils';
+import { isSameBlockData } from '../../utils/blocks';
+import { PopoverItemDefaultParams } from '../../utils/popover';

 /**
 * Block Converter
 *
 * @todo Make the Conversion Toolbar no-module but a standalone class, like Toolbox


this todo is not actual now

neSpecc · 2024-06-09T21:03:12Z

src/components/modules/toolbar/conversion.ts

-     */
-    this.addTools();
+    conversionEntries.forEach(([toolName, tool]) => {
+      const conversionConfig = tool.conversionConfig;

-    /**
-     * Prepare Flipper to be able to leaf tools by arrows/tab
-     */
-    this.enableFlipper();
-
-    $.append(this.nodes.wrapper, label);
-    $.append(this.nodes.wrapper, this.nodes.tools);
-
-    return this.nodes.wrapper;
-  }
-
-  /**
-   * Deactivates flipper and removes all nodes
-   */
-  public destroy(): void {
-    /**
-     * Sometimes (in read-only mode) there is no Flipper
-     */
-    if (this.flipper) {
-      this.flipper.deactivate();
-      this.flipper = null;
-    }
-
-    this.removeAllNodes();
-  }
-
-  /**
-   * Toggle conversion dropdown visibility
-   *
-   * @param {Function} [togglingCallback] — callback that will accept opening state
-   */
-  public toggle(togglingCallback?: (openedState: boolean) => void): void {
-    if (!this.opened) {
-      this.open();
-    } else {
-      this.close();
-    }
-
-    if (_.isFunction(togglingCallback)) {
-      this.togglingCallback = togglingCallback;
-    }
-  }
-
-  /**
-   * Shows Conversion Toolbar
-   */
-  public open(): void {
-    this.filterTools();
-
-    this.opened = true;
-    this.nodes.wrapper.classList.add(ConversionToolbar.CSS.conversionToolbarShowed);
-
-    /**
-     * We use RAF to prevent bubbling Enter keydown on first dropdown item
-     * Conversion flipper will be activated after dropdown will open
-     */
-    window.requestAnimationFrame(() => {
-      this.flipper.activate(this.tools.map(tool => tool.button).filter((button) => {
-        return !button.classList.contains(ConversionToolbar.CSS.conversionToolHidden);
-      }));
-      this.flipper.focusFirst();
-      if (_.isFunction(this.togglingCallback)) {
-        this.togglingCallback(true);
+      /**
+       * Skip tools without «import» rule specified
+       */
+      if (!conversionConfig || !conversionConfig.import) {


its better to use the isBlockConvertable() util

neSpecc · 2024-06-09T21:04:28Z

src/components/modules/toolbar/conversion.ts

        /**
-         * Skip tools without «import» rule specified
+         * Skip tools that don't pass 'toolbox' property


seems like the comment is not actual because you're iterating not over tools, but over toolbox items inside a tool

neSpecc · 2024-06-09T21:29:16Z

src/components/modules/toolbar/inline.ts

  }

  /**
   * Shows Inline Toolbar
   */
-  private open(): void {
+  private async open(): Promise<void> {


I'm not sure, but seems like somewhere can be a memory leak. Try to open/close it many times in a row and take a look on the profiler. Nodes and listeners are growing in time

neSpecc · 2024-06-09T22:10:21Z