Skip to content
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

Optimized initial toolbar render with toolbar grouping enabled #7656

Merged
merged 11 commits into from
Jul 22, 2020

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Jul 21, 2020

Suggested merge commit message (convention)

Other (ui): Improved toolbar rendering time when multiple items are added or removed at once (e.g. during editor initialization). Closes #6194.

Fix (ui): Removing the first hidden toolbar button will no longer throw an exception. Closes #7655.


Additional information

While working on it I spotted #7655 and decided to fix it while porting old listeners.

@mlewand mlewand requested a review from oleq July 21, 2020 08:03
@@ -302,7 +302,7 @@ export default class ToolbarView extends View {
console.warn( attachLinkToDocumentation(
'toolbarview-item-unavailable: The requested toolbar item is unavailable.' ), { name } );
}
} );
} ).filter( item => item !== undefined ) );
Copy link
Member

Choose a reason for hiding this comment

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

Is there any test for this one? Why is it needed anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case factory has no matching button then an undefined is returned (from the callback above). It happens if you misspell button name in the configuration.

There's at least one existing test that will fail if it's not handled correctly:

it( 'warns if there is no such component in the factory', () => {
const items = view.items;
const consoleWarnStub = sinon.stub( console, 'warn' );
view.fillFromConfig( [ 'foo', 'bar', 'baz' ], factory );
expect( items ).to.have.length( 2 );
expect( items.get( 0 ).name ).to.equal( 'foo' );
expect( items.get( 1 ).name ).to.equal( 'bar' );
sinon.assert.calledOnce( consoleWarnStub );
sinon.assert.calledWithExactly( consoleWarnStub,
sinon.match( /^toolbarview-item-unavailable/ ),
{ name: 'baz' }
);
} );

packages/ckeditor5-ui/tests/toolbar/toolbarview.js Outdated Show resolved Hide resolved
packages/ckeditor5-ui/tests/toolbar/toolbarview.js Outdated Show resolved Hide resolved
@mlewand mlewand requested a review from oleq July 21, 2020 17:16
@oleq oleq merged commit 266dfda into master Jul 22, 2020
@oleq oleq deleted the i/6194-collection-getall branch July 22, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants