-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(inline-tools): Inline tools rendered as popover #2718
base: next
Are you sure you want to change the base?
Conversation
c8479e7
to
94232ea
Compare
/** | ||
* 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)); |
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.
Would it work on touch devices?
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.
checked, works
const totalLeftOffset = this.offsetLeft + itemOffsetLeft; | ||
|
||
nestedPopoverEl.style.setProperty( | ||
'--trigger-item-left', |
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 wonder if it would be better to put all the properties into some constants or enum?
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.
added enum with variables
Changed |
2b73b7c
to
834391c
Compare
57d26c3
to
b88e184
Compare
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.
@@ -69,7 +69,6 @@ export default class DragNDrop extends Module { | |||
private async processDrop(dropEvent: DragEvent): Promise<void> { | |||
const { | |||
BlockManager, | |||
Caret, |
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.
Caret is used below. Probably, you should import the new caret position enum
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.
fixed
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 |
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.
this todo is not actual now
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.
fixed
*/ | ||
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) { |
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.
its better to use the isBlockConvertable()
util
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.
fixed. Though isBlockConvertable()
is not suitable for this case, here we operate tools, not blocks. Added isToolConvertable()
util
/** | ||
* Skip tools without «import» rule specified | ||
* Skip tools that don't pass 'toolbox' property |
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.
seems like the comment is not actual because you're iterating not over tools, but over toolbox items inside a tool
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.
fixed
} | ||
|
||
/** | ||
* Shows Inline Toolbar | ||
*/ | ||
private open(): void { | ||
private async open(): Promise<void> { |
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.
items: item.children, | ||
nestingLevel: this.nestingLevel + 1, | ||
}); | ||
|
||
this.emit(PopoverEvent.OpenNestedPopover, { | ||
/** Inputs potentially can be auto focused */ |
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.
example would be helpful here
|
||
nestedPopoverEl.style.setProperty('--trigger-item-top', topOffset + 'px'); | ||
nestedPopoverEl.style.setProperty('--nesting-level', this.nestedPopover.nestingLevel.toString()); | ||
this.setTriggerItemPositionProperty(nestedPopoverEl, item); |
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.
maybe simpler?
this.setTriggerItemPositionProperty(nestedPopoverEl, item); | |
this.setTriggerItemPosition(nestedPopoverEl, item); |
nestedPopoverEl.style.setProperty('--nesting-level', this.nestedPopover.nestingLevel.toString()); | ||
this.setTriggerItemPositionProperty(nestedPopoverEl, item); | ||
|
||
nestedPopoverEl.style.setProperty(CSSVariables.NestingLevel, this.nestedPopover.nestingLevel.toString()); |
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.
add doc please, why CSS should know about nesting level
this.setTriggerItemPositionProperty(nestedPopoverEl, item); | ||
|
||
nestedPopoverEl.style.setProperty(CSSVariables.NestingLevel, this.nestedPopover.nestingLevel.toString()); | ||
nestedPopoverEl.classList.add(css.getPopoverNestedClass(this.nestedPopover.nestingLevel)); |
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.
it is also not clear why we need this class. docs would be helpful
private handleHover(event: Event): void { | ||
const item = this.getTargetItem(event); | ||
private addSearch(): void { | ||
this.search = new SearchInput({ |
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.
is there any destroy()
call for search?
|
||
/** | ||
* Sets CSS variable with position of item near which nested popover should be displayed. | ||
* Is used for correct positioning of the nested popover |
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.
Is used for correct positioning of the nested popover
describe what is the "correct positioning". And describe why we need to this.offsetLeft + itemOffsetLeft
} | ||
|
||
/** | ||
* Overrides default item click handling to handle nested popover closing correctly |
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.
describe what does it mean "closing correctly"
/** | ||
* When nested popover should opens | ||
*/ | ||
OpenNestedPopover = 'open-nested-popover', |
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.
Its better to name events in a past form
src/styles/inline-toolbar.css
Outdated
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 think there are many unused styles that should be removed
New Inline Tools
Inline tools now use popover as well as Toolbox and Block Tunes 💪
What's done
PopoverInline
class added. It extendsPopoverDesktop
but makes it horizontal (and adds few features)ToDo