t/263: Implemented public show and hide methods in the ContextualToolbar plugin #270
Conversation
…bar plugin. Closes #263. Extended enableToolbarKeyboardFocus helper with beforeFocus and afterBlur callbacks.
| * Removes panel from the {@link: #_balloon}. | ||
| * | ||
| * @private | ||
| * Removes the toolbar from the {@link: #_balloon} which hides it. |
There was a problem hiding this comment.
Don't reference private stuff in public docs.
| // of lines of text. Using getClientRects() allows us to browse micro–ranges | ||
| // that would normally make up the bounding client rect. | ||
| const rangeRects = editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ).getClientRects(); | ||
| const range = editingView.selection.getFirstRange(); |
There was a problem hiding this comment.
How's this change related to this PR?
There was a problem hiding this comment.
It's strongly related and it's all about https://github.com/ckeditor/ckeditor5-utils/issues/168.
When we first implemented CT it was for non–collapsed selections only and hence it was immune to some weird DOMRange vs. ClientRect issues. Now that it gains public show() and hide(), it can be displayed for collapsed selections and it means it broke a lot.
So the following change
- // getBoundingClientRect() makes no sense when the selection spans across number
- // of lines of text. Using getClientRects() allows us to browse micro–ranges
- // that would normally make up the bounding client rect.
- const rangeRects = editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ).getClientRects();
+ const range = editingView.selection.getFirstRange();
+ const rangeRects = Rect.getDomRangeRects( editingView.domConverter.viewRangeToDom( range )uses the power of the Rect class to avoid weird bugs, keep us safe and DRY.
| @@ -24,7 +24,9 @@ export default function enableToolbarKeyboardFocus( { | |||
| origin, | |||
| originKeystrokeHandler, | |||
| originFocusTracker, | |||
| toolbar | |||
| toolbar, | |||
| beforeFocus, | |||
There was a problem hiding this comment.
Missing documentation for these two new properties.
| @@ -33,7 +35,12 @@ export default function enableToolbarKeyboardFocus( { | |||
| // Focus the toolbar on the keystroke, if not already focused. | |||
| originKeystrokeHandler.set( 'Alt+F10', ( data, cancel ) => { | |||
| if ( originFocusTracker.isFocused && !toolbar.focusTracker.isFocused ) { | |||
| if ( beforeFocus ) { | |||
There was a problem hiding this comment.
As I commented in the other PR. Calling this callback beforeFocus() makes it a bit hard to read. It took me a couple of minutes to understand this. Unfortunately... now it makes perfect sense, so I can't find some new ideas :D.
…) helper and ContextualToolbar plugin.
Suggested merge commit message (convention)
Other: Implemented public show and hide methods in the ContextualToolbar plugin. Closes ckeditor/ckeditor5#5383.