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

Make BlockToolbar work with empty configuration. #534

Merged
merged 2 commits into from
Dec 20, 2019
Merged

Make BlockToolbar work with empty configuration. #534

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea02023
Make BlockToolbar work with empty configuration.
jodator Dec 16, 2019
53d35ff
Remove asynchronous test code from expect().
jodator Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/toolbar/block/blocktoolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export default class BlockToolbar extends Plugin {
*/
afterInit() {
const factory = this.editor.ui.componentFactory;
const config = this.editor.config.get( 'blockToolbar' );
const config = this.editor.config.get( 'blockToolbar' ) || [];

this.toolbarView.fillFromConfig( config, factory );

Expand Down
23 changes: 23 additions & 0 deletions tests/toolbar/block/blocktoolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,17 @@ describe( 'BlockToolbar', () => {
expect( BlockToolbar.pluginName ).to.equal( 'BlockToolbar' );
} );

it( 'should work with empty config', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I checked out src/ to master and expected this test to fail, but it didn't. The other one did, but then why didn't this one? Maybe expect() doesn't handle async functions?

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 check it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... that was not wise for me 😥.

With promises we have three choices:

  1. Expecting to not fail not explicitly (anyway test must pass)

    editor = await ClassicTestEditor.create( element, {
  plugins: [ BlockToolbar ]
} );
// Optional comment: The test must not throw or rename the test
//  to "should not throw when empty config is provided"

  2. Explicitly fail on error

    try {
	editor = await ClassicTestEditor.create( element, {
		plugins: [ BlockToolbar ]
	} );
} catch ( error ) {
	expect.fail( 'Error should not be thrown.' );
}

  3. Use chai-as-promised:

    expect( async () => {
	editor = await ClassicTestEditor.create( element, {
		plugins: [ BlockToolbar ]
	} );
} ).to.evantually.not.throw();

I'm for 1. But every other way works (3 needs update in the dev-tests to add chai-as-promised.

Copy link
Member

Choose a reason for hiding this comment

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

  1. is fine for me too.

// Remove default editor instance.
await editor.destroy();

expect( async () => {
editor = await ClassicTestEditor.create( element, {
plugins: [ BlockToolbar ]
} );
} ).to.not.throw();
} );

describe( 'child views', () => {
describe( 'panelView', () => {
it( 'should create a view instance', () => {
Expand Down Expand Up @@ -213,6 +224,18 @@ describe( 'BlockToolbar', () => {

expect( blockToolbar.buttonView.tooltip ).to.be.false;
} );

it( 'should hide the #button if empty config was passed', async () => {
// Remove default editor instance.
await editor.destroy();

editor = await ClassicTestEditor.create( element, {
plugins: [ BlockToolbar ]
} );

const blockToolbar = editor.plugins.get( BlockToolbar );
expect( blockToolbar.buttonView.isVisible ).to.be.false;
} );
} );
} );

Expand Down