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

Fix deprecated getEntity (fixes #67) #71

Merged
merged 3 commits into from
Mar 23, 2017
Merged

Fix deprecated getEntity (fixes #67) #71

merged 3 commits into from
Mar 23, 2017

Conversation

mikaelwaltersson
Copy link
Contributor

WARNING: DraftEntity.get will be deprecated soon! Please use "contentState.getEntity" instead.

Copy link
Owner

@sstur sstur left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! I've got a few questions here..

);
this.inlineStyles = inlineStyles;
this.styleOrder = styleOrder;
}
Copy link
Owner

Choose a reason for hiding this comment

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

What's the difference between the code on the left and the code on the right? If there are no changes can you rebase this PR so they are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No change really, must be some spacing or something changed just be opening and saving the file in my editor. I couldn't manage to unstage the hunk either using SourceTree.

export default function stateToHTML(content: ContentState, options: ?Options): string {
return new MarkupGenerator(content, options).generate();
export default function stateToHTML(content: ContentState, options: ?Options): string {
return new MarkupGenerator(content, options).generate();
Copy link
Owner

Choose a reason for hiding this comment

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

Also, what is the difference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No change really, must be some spacing or something changed just be opening and saving the file in my editor. I couldn't manage to unstage the hunk either using SourceTree.

@@ -341,7 +340,7 @@ class MarkupGenerator {
}
return content;
}).join('');
let entity = entityKey ? Entity.get(entityKey) : null;
let entity = entityKey ? this.contentState.getEntity(entityKey) : null;
Copy link
Owner

Choose a reason for hiding this comment

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

This is the main change of this PR, right? This fixes the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, Entity.get is deprecated as of 0.10 and replaced by contentState.getEntity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 have removed the accidental trailing whitespace changes now.

WARNING: DraftEntity.get will be deprecated soon! Please use "contentState.getEntity" instead
@gbbr
Copy link

gbbr commented Feb 28, 2017

👍

@svnm
Copy link

svnm commented Mar 2, 2017

Have tested and this works well for me in Draft 0.10.0 👍

@blade254353074
Copy link

How is it now?

sstur
sstur previously requested changes Mar 22, 2017
Copy link
Owner

@sstur sstur left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. My only concern is that this will NOT work with older versions draft-js which do not support contentState.getEntity()

We should create a helper function with feature detection:

function getEntity(contentState, entityKey) {
  return contentState.getEntity ? contentState.getEntity(entityKey) : Entity.get(entityKey);
}

Then you can change line 343 to:

let entity = entityKey ? getEntity(this.contentState, entityKey) : null;

@sstur sstur changed the title Fix issue #67 Fix deprecated getEntity (fixes #67) Mar 22, 2017
…ions of draft-js as per sstur's suggestion
@mikaelwaltersson
Copy link
Contributor Author

Added your improvements for backward compatibility

@sstur sstur dismissed their stale review March 23, 2017 04:25

The code has been updated.

@@ -377,6 +376,10 @@ class MarkupGenerator {

}

function getEntity(contentState, entityKey) {
return contentState.getEntity ? contentState.getEntity(entityKey) : Entity.get(entityKey);
Copy link
Owner

Choose a reason for hiding this comment

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

Entity is not defined, is it?
I think because you removed line 7.

@sstur
Copy link
Owner

sstur commented Mar 23, 2017

Please run npm test before submitting. This is partly my fault because I should setup TravisCI to automatically run test on all PRs. This is the error:

screenshot 2017-03-23 12 41 56

@mikaelwaltersson
Copy link
Contributor Author

Sorry for that, weren't able to install all the tools necessary to run the tests and unfortunately I was to busy to look into it any further.

screen shot 2017-03-23 at 4 59 46 pm

@sstur
Copy link
Owner

sstur commented Mar 23, 2017

Thanks!

@sstur sstur merged commit 090569e into sstur:master Mar 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants