feat: implement Block Tunes#157
Conversation
Unit Tests
Mutation Tests
|
gohabereg
left a comment
There was a problem hiding this comment.
Great work! Going through the PR started my train of thought on the Tunes overall. I invite you all for a discussion @bettysteger @neSpecc @e11sy
Below is just my stream of consciousness, hope it makes sense
TL;DR
I suggest to remove BlockTune as an entity and just use Plugins. That could be possible with 3 core changes:
- UI Plugin would have an API
1.1. API would have a capability to add a button to the Settings popover
1.2. API would also have a capability to change a Block appearance via a wrapper (the same way it's done in v2 now)- Instead of
tunesproperty in the BlockNode, we'd haveplugins. Any Plugin would be able to save data to the Model and retrieve it.- "Tune" becomes a Plugin that utilises UI Plugin APIs and of course Core APIs to change the Model
The "Tune" term than becomes just a UI term, not an entity. Therefore there's no need to have a separate BlockTuneAdapter.
The purpose of the Adapter is to connect the Tune instance to the Model. In the PR there's no such connection — Tune updates the model through the Core API rather than Adapter and Adapter doesn't handle Model updates (in this PR, because there's no need for internal Tunes).
Comparing TuneAdapter to FormattingAdapter, CaretAdapter and BlockToolAdapters: these 3 adapters are converting user input (typing, selection, applying inline formatting) to the Model data. It could be done without Adapters, Tools could just work with the DOM directly and update the Model via Core API. But Adapters remove boilerplate and handle complex DOM operations, so a Tool developer doesn't need to. With the Tunes there is no complex DOM operations or converting complex user input into the Model data. Ultimately, Tune is just a UI layer for the Core API calls + some of them might carry some data (e.g. Anchors Tune, or Footnotes Tune).
That kinda makes a Tune just a Plugin with the ability to add a button — maybe that's the way to go? There are use-cases when we expect the Tune to change editor's zone appearance. In v2 it's done via a Block wrapper — maybe we just add this capability to UI Plugin instead of having a separate "Tune" entity.
So the final design would look like:
- UI being a Plugin would provide capabilities to change Block's appearance with a wrapper
- Any Plugin can interact with another Plugin via it's public API (see a ticket on the project board to expose Plugins API)
- Any Plugin can add a button into the Settings section of the UI
- Tunes become just Plugins interacting with UI plugin.
- In the Model structure instead of "tunes" property in the BlockNode we introduce something like "plugins" or "pluginsData" and add a capability for each Plugin to change that data through the Core API.
| this.#popover.addItem({ | ||
| name, | ||
| title: tune.title, | ||
| icon: tune.icon, | ||
| closeOnActivate: true, | ||
| isDisabled: tune.isDisabled?.(), | ||
| onActivate: () => { | ||
| tune.activate?.(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I think a Tune (or a Plugin if we go with the approach I suggest) can return the configuration (or partial configuration) the same way InlineTool does.
A question however — is it better to call some instance method onActivate or instance would just provide onActivate in the config.
| /** | ||
| * Deletes the block this tune is attached to | ||
| */ | ||
| public activate(): void { |
There was a problem hiding this comment.
In v2 this tune has confirmation state as well (could be done via Popover)
#150