-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(toolbar): toolbar refactored and ui improved #1815
Conversation
- UI module triggers 'block-hovered' event - Toolbar uses 'block-hovered' for appearing - `currentBlock` setter added to the BlockManager - (reactangle-selection): the throttling added to the mousemove and scroll handlers - `getBlockIndex` method added to the Api - (api-blocks): toolbar moving logic removed from `blocks.move()` and `blocks.swap()` methods. Instead, MoveUp and MoveDown tunes uses Toolbar API
- Toolbox became a standalone class from the editor module. It can be accessed only via the owner (the Toolbar module) - (api.blocks) the insert() method now has the `replace` param. Also, it returns inserted Block API now.
…yId()` method added
@@ -109,6 +109,9 @@ export default class MoveDownTune implements BlockTune { | |||
/** Change blocks positions */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolbar moving logic removed from the api.blocks.move()
method, so we do it manually.
/** | ||
* Prevent caret jumping (to last block) when clicking between blocks | ||
*/ | ||
lastBlockBottomCoord < clickedCoord; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have improved the isClickedBottom
statement:
some blocks can have margins, so when a user clicks on such margins between .ce-block
, caret was jumping to the bottom of the document. For now, it checks the exact coordinate of the last block bottom.
this.api.toolbar.open(); | ||
this.api.toolbar.toggleBlockSettings(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks not really convenient, maybe we can at least use just one method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
this.plusButton.show(); | ||
} else { | ||
this.plusButton.hide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to move plus button into toolbox as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that it would be better. Toolbox is the list of buttons. It is about to become vertical. And it can be appeared in several ways, not only by Plus Button. And the Plus Button is just a part of Toolbar UI.
*/ | ||
let blockHoveredEmitted; | ||
|
||
this.readOnlyMutableListeners.on(this.nodes.redactor, 'mousemove', _.throttle((event: MouseEvent | TouchEvent) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not mouseenter
and mousemove
. Also would be great to check on pointer devices and mobile devices with connected touchpad/mouse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mouseenter
would have to be bound to every Block, and there can be a large amount of them. I think it's better to use the single event on the redactor
node.
Also would be great to check on pointer devices and mobile devices with connected touchpad/mouse
I will test the RC version in CodeX Docs, so I will try to use various hardware.
src/components/ui/toolbox.ts
Outdated
this.onOpen = onOpen; | ||
this.onClose = onClose; | ||
this.onBlockAdded = onBlockAdded; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case API would grow, might be better to just extend EventEmitter interface and provide enum of possible events
toolbox.on(ToolboxEvents.Close, () => {});
toolbox.on(ToolboxEvents.Open, () => {});
Using typescript we can strictly type that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
toolName, | ||
undefined, | ||
undefined, | ||
index, | ||
undefined, | ||
currentBlock.isEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go with object here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided not to change the interface of the existed API method. Should I create an overload for that?
this.toolboxInstance = new Toolbox({ | ||
api: this.Editor.API.methods, | ||
tools: this.Editor.Tools.blockTools, | ||
shortcutsScopeElement: this.Editor.UI.nodes.redactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to provide some nodes from Editor API
editorAPI.nodes.redactor
// or
editorAPI.ui.redactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This PR introduces several updates of the Toolbar code and UI.
This is the first part of updates due to upcoming feature called "Unified menu".
List of changes
block-hovered
eventblock-hovered
for appearingcurrentBlock
setter added to the BlockManager. We use it to set the Current Block on clicks on the BT Toggler or the Plus Button (and some other cases as well)mousemove
andscroll
handlersgetBlockIndex()
method addedblocks.move()
andblocks.swap()
methods. Instead, you should use Toolbar API (it was used by MoveUp and MoveDown tunes, they were updated).insert()
method now has thereplace: boolean
parameterinsert()
method now returns the insertedBlock API
on()
method now returns the listener id.offById()
method addedBlock Settings
will be refactored the same way in the next updates.UiApi
section was added. It allows accessing some editor UI nodes and methods.Related editor-js/header#72