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

i/5597: Set inline toolbar's max-width #65

Merged
merged 25 commits into from
Feb 26, 2020
Merged

i/5597: Set inline toolbar's max-width #65

merged 25 commits into from
Feb 26, 2020

Conversation

panr
Copy link
Contributor

@panr panr commented Jan 14, 2020

Suggested merge commit message (convention)

Feature: The inline editor toolbar should group items when its width exceeds the editable’s width (see ckeditor/ckeditor5#5597).

BREAKING CHANGE: From now on, the inline toolbar groups overflowing items by default. This behaviour can be disabled via the editor config by setting the config.toolbar.shouldNotGroupWhenFull: true option.


Master PR: ckeditor/ckeditor5-ui#536
CI https://github.com/ckeditor/ckeditor5/compare/i/5597?expand=1

@panr panr added the pr:sub label Jan 14, 2020
@coveralls
Copy link

coveralls commented Jan 14, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling e6f4e31 on i/5597 into 18e67c1 on master.

@panr
Copy link
Contributor Author

panr commented Jan 14, 2020

@oleq I see that I need some help with this...

I'm not sure about the place when I implemented this fix/feature, should it be inside _initToolbar()#on:update? 🤔 If so, we probably don't want to update ...style.maxWidth each time something has changed inside editor... That's why I put it inside another condition (tests are needed)...

@panr
Copy link
Contributor Author

panr commented Jan 15, 2020

Coverage decreased (-12.6%) to 87.413% when pulling 28b64ee on i/5597 into 02763b0 on master.

coverage/coveralls — Coverage increased (+12.6%) to 100.0%

yeah

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

I briefly checked your approach and here are my thoughts:

  • As for now the edutor.ui#update is fired only when the content of the editor changes. It won't react when the window is being resized which means it is not very useful for this case. We have plans to improve it, though.
    • Knowing this, I assume your solution is static. Once set, the maxWidth never changes. I think it should. Focus any editor here and resize the browser window – I think the maxWidth should be dynamic.
  • I think what we need here is:
    • A DOM resize observer (in InlineEditorUI or InlineEditorUIView, this needs to be checked, probably the former), which can be obtained using our API.
    • A public observable property ToolbarView#maxWidth bound to toolbar element's style in ToolbarView#setTemplate().
    • Resizer observer watching the editableElement's width and setting the corresponding ToolbarView#maxWidth of the toolbar, if the editable is narrower than the toolbar. See the source of the ToolbarView for an example of the observer.
    • A support for config.toolbar.shouldNotGroupWhenFull in inline/ballon/etc. creators (controlling whether to enable or disable the above). ATM only supported in classic/decoupled.

@panr panr requested a review from oleq January 24, 2020 09:38
@panr
Copy link
Contributor Author

panr commented Jan 24, 2020

@oleq please remember to check also this PR — ckeditor/ckeditor5-ui#536

⚠️I spotted an issue when you increase the width of the editable, toolbar doesn't unwrap grouped items. It's because ATM it doesn't "know" about editable width... I'll try to fix this by passing down the editable's width to the toolbar as an optional argument...

EDIT: OK, I've been wrong. It looks like we need to rethink the logic of grouping behaviour inside _updateGrouping().

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

The approach in this PR is OK. I pointed out a few issues and suggested some improvements.

Items grouping and max-width

I spotted an issue when you increase the width of the editable, toolbar doesn't unwrap grouped items.

You're right, you can reproduce it using

const firstEditor = editors[ '#editor-1'];
let width = 200;

setInterval( () => {
    width += 10;

    firstEditor.editing.view.change( writer => {
        const viewEditableRoot = firstEditor.editing.view.document.getRoot();

        writer.setAttribute( 'style', `width:${ width }px`, viewEditableRoot );
    } );
}, 100 );

in http://localhost:8125/ckeditor5-editor-inline/tests/manual/inlineeditor.html.

To fix that we need DynamicGrouping#_enableGroupingOnMaxWidthChange(); in the related ckeditor5-ui PR with this simple implementation:

_enableGroupingOnMaxWidthChange( view ) {
	view.on( 'change:maxWidth', () => {
		this._updateGrouping();
	} );
}

Kapture 2020-01-24 at 16 50 32

Copy link
Contributor Author

@panr panr left a comment

Choose a reason for hiding this comment

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

Fixes are ready 👍

@panr
Copy link
Contributor Author

panr commented Jan 29, 2020

To fix that we need DynamicGrouping#_enableGroupingOnMaxWidthChange(); in the related ckeditor5-ui PR with this simple implementation:

Added here ckeditor/ckeditor5-ui@bb97620

I think that after switch the new ResizeObserver class (ckeditor/ckeditor5#6145), we can rewrite those two methods (ckeditor/ckeditor5-ui@bb97620#diff-f06024eea947963b77dc470d21fac3d9R672-R715), merge them and also add maxWidth to the Classic and Decoupled Editor. WDYT?

@panr panr requested a review from oleq January 30, 2020 09:56
@panr panr requested a review from oleq February 6, 2020 07:55
@panr panr requested a review from oleq February 26, 2020 12:08
@oleq oleq merged commit 1c5746c into master Feb 26, 2020
@oleq oleq deleted the i/5597 branch February 26, 2020 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants