Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Feature: Introduced BlockToolbar plugin. #392

Merged
merged 57 commits into from May 30, 2018
Merged

Feature: Introduced BlockToolbar plugin. #392

merged 57 commits into from May 30, 2018

Conversation

@coveralls
Copy link

coveralls commented May 18, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 55b6ab3 on t/391 into 6508ba2 on master.

@pjasiun
Copy link

pjasiun commented May 22, 2018

Does line-height: 1em is not needed anymore?

@oskarwrobel
Copy link
Contributor Author

There is no clear information in the manual test that there should be no balloon toolbar next to header 1. I had to check source code to ensure it is not a bug.

Button should be displayed next to all headers. Button is not displayed next to image because there are no available options in toolbar for changing image.

@oskarwrobel
Copy link
Contributor Author

oskarwrobel commented May 22, 2018

Does line-height: 1em is not needed anymore?

No it's not.
See https://github.com/ckeditor/ckeditor5-ui/pull/392/files#diff-5c933689b1a22af6c577f3f83c5a82b3R263.

const toPx = toUnit( 'px' );

/**
* The block button view class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs. What is this button? What's its purpose? What's its behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*
* @type {module:ui/toolbar/toolbarview~ToolbarView}
*/
this.toolbarView = new ToolbarView( editor.locale );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely missing

this.toolbar.extendTemplate( {
	attributes: {
		class: [
			// https://github.com/ckeditor/ckeditor5-editor-inline/issues/11
			'ck-toolbar_floating'
		]
	}
} );

just like in the InlineEditorUIView and BalloonToolbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@oleq
Copy link
Member

oleq commented May 28, 2018

The button fails to reposition as the target is being scrolled inside an overflow: scroll container.

kapture 2018-05-28 at 16 14 43

Should we handle this case? If so, even if we used the BalloonPanelView as button's parent, we're gonna fall into https://github.com/ckeditor/ckeditor5-ui/issues/181 anyway.

*/

/**
* @module ui/toolbar/block/view/blockbuttonview
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this view subfolder? It's unlikely another view will be there and it makes the structure unnecessarily complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move this view to ui/toolbar/block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -70,11 +70,11 @@ describe( 'BlockToolbar', () => {
} );

it( 'should add panelView to ui.focusTracker', () => {
expect( editor.ui.focusTracker.isFocused ).to.false;
expect( editor.ui.focusTracker.isFocused ).to.be.false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

@oskarwrobel
Copy link
Contributor Author

The button fails to reposition as the target is being scrolled inside an overflow: scroll container.
Should we handle this case? If so, even if we used the BalloonPanelView as button's parent, we're gonna fall into #181 anyway.

I think we should handle this case and have one common problem with floating stuff + scroll.

@oskarwrobel
Copy link
Contributor Author

Ticket about repositioning on scroll issue https://github.com/ckeditor/ckeditor5-ui/issues/398.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants