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

T/47 #48

Merged
merged 7 commits into from
Apr 19, 2019
Merged

T/47 #48

merged 7 commits into from
Apr 19, 2019

Conversation

elszczepano
Copy link
Member

@elszczepano elszczepano commented Apr 18, 2019

Suggested merge commit message (convention)

Fix: Changed the way of data initialization. Instead of using setData(), the content is set via innerHTML. Closes #47.


@coveralls
Copy link

coveralls commented Apr 18, 2019

Pull Request Test Coverage Report for Build 65

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 53: 0.0%
Covered Lines: 26
Relevant Lines: 26

💛 - Coveralls

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

@@ -104,19 +104,6 @@ describe( 'CKEditor Component', () => {
expect( vm.value ).to.equal( '' );
} );

it( 'should set the initial data', done => {
Copy link
Member

Choose a reason for hiding this comment

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

This test should not be removed. We need to be sure that the specified data has been passed to the editor.

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.

Some questions and mostly code-style concerns.

BTW: The PR title is missing some text.

src/ckeditor.js Outdated Show resolved Hide resolved
it( 'should set the initial data', done => {
const setDataStub = sandbox.stub( MockEditor.prototype, 'setData' );
const { wrapper } = createComponent( {
it( 'should set the initial data by using innerHTML, not by "setData()"', done => {
Copy link
Member

Choose a reason for hiding this comment

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

Missing link to the issue.

tests/ckeditor.js Outdated Show resolved Hide resolved
return createElement( this.tagName );
return createElement(this.tagName, {
domProps: {
innerHTML: this.value || ''
Copy link
Member

Choose a reason for hiding this comment

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

Does this work when tagName is textarea?

Copy link
Member

Choose a reason for hiding this comment

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

I'm worried because setting the value of an input like this works only once (the second one will be ignored). In theory, we do it only once but still, this is not the right way IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Also... does this whole innerHTML thing work when a content with/without entities is passed as a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've built a sample with tagName set to textarea. The editor seems to work properly, no error occurs on init and on destroy.

tests/ckeditor.js Outdated Show resolved Hide resolved
@oleq oleq merged commit 6f821fa into master Apr 19, 2019
@oleq oleq deleted the t/47 branch April 19, 2019 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialize data via setData() doesn't work properly
5 participants