search

sstur / react-rte

Pure React rich text WYSIWYG editor based on draft-js.
https://react-rte.org
ISC License
2.86k stars 430 forks source link

Fix width and height attribute for img #389

Closed xiaohanzhang closed 1 year ago

xiaohanzhang commented 3 years ago

Fix #388

xiaohanzhang commented 3 years ago

@sstur , is there any feedback on this?

xiaohanzhang commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sstur commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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.