Fix width and height attribute for img

[xiaohanzhang]

Fix #388

• Bump draft-js-utils version
• Fix ImageSpan width and height type

[xiaohanzhang]

@sstur , is there any feedback on this?

[sstur]

Hey @xiaohanzhang, thanks for the PR.

Could you help me understand exactly what bug/issue this fixes?

[sstur]

Hmm, I'm not clear on why we need this logic; What's the case where one of these would be NaN?

If somehow a non-number (or invalid number) could sneak into the state for width/heigh, I'm assuming it's up on line 35. Should we do the validation there instead?

[xiaohanzhang]

<img/> support both number and string for width and height, so we need this logic to handle both case:

<img width="100" height="100"/> 
<img width="100px" height="100px"/>

And I agree it is better to do validation on line 35. I was trying to limit the PR size, I thought change on line 66 is safer.
Do you want me move it to line 35?

[xiaohanzhang]

For example: <img width="100" height="100"/>
It will resolve into string "100" in entity.getData() on line 35, so we will get const imageStyle = {width: '100', height: '100'} on line 72.
Then React will ignore it, because '100' is not a valid value for width and height.

[sstur]

OK, I think I get it now, so we're intending to support string for width/height prop.

Is this only so that we can support an integer pixel value that's cast as a string, e.g. "123" or are you intending to support a string like "80%"?

If it's the first case we should convert it to a number before we put it in state.

If it's the second case then we need to update the type from number to number | string on line 23-24, and yes, we should do some validation on line 35 also.

[xiaohanzhang]

I think this will depends on your design decision. html4 support both percentage and number, and html5 only support number. So it is good to support both, but still make sense to only support html5. A full support for html4 could be more complex, since we need handle both background and width in imageStyle.

But either way, we will need to fix the "123" case, because currently edit a content with image doesn't work at all. I couldn't figure out any way to keep image size after edit the content. The image will auto save as"123" instead of 123.

[sstur]

OK, let's ignore percentages for now, since it's out of scope for this fix.

Instead let's just cast the string to a number at the time that we put it in state and then we can land this.

Thanks!

[xiaohanzhang]

@sstur Should I fix auto build error, I think it is unrelated.

[sstur]

@xiaohanzhang, can you make sure you're not checking in the build artifacts. It shows there are 43 files changed in this PR, which isn't right. Nothing from dist/ should show up here.

[xiaohanzhang]

@sstur Sorry, it's been two years, I thought this pr was abandoned. I reverted my last commit.
Is it possible to merge this PR?

[sstur]

Yes, to be honest this project is long outdated and not being maintained. Not sure there's much value in updating dependencies and making a release. But I'll merge this one and test it out.