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

merged 2 commits into from
Dec 20, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Dec 16, 2019

Suggested merge commit message (convention)

Fix: Make BlockToolbar work with an empty configuration. Closes ckeditor/ckeditor5#5980.


Additional information

  • As discussed in config may not exist #532 the block toolbar should work with empty toolbar configuration as other features use this pattern.

@@ -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.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

The code is good but I'd like to clarify why one of the tests doesn't fail on master.

@Reinmar Reinmar merged commit 1e05098 into master Dec 20, 2019
@Reinmar Reinmar deleted the i/5980 branch December 20, 2019 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Balloon toolbar doesn't assume empty toolbar.
2 participants