fix article doesn't display in Safari

sstur / draft-js-utils

DraftJS: import/export ContentState to and from HTML/Markdown

ISC License

883 stars 233 forks source link

fix article doesn't display in Safari #261

Closed yuxin1234 closed 1 year ago

yuxin1234 commented 2 years ago

Resolves #260

sstur commented 2 years ago

HI @yuxin1234, thanks for the contribution!

Could you please add one or more tests to ensure this does what's expected. It's hard to tell by looking at Regex if it will work right, so adding some tests would be hugely helpful. Ideally a test for such a fix should fail before this patch but pass afterwards.

Note: I just pushed a commit to master to enable automated tests via Github Actions, so please do a git pull.

Thanks!

yuxin1234 commented 2 years ago

@sstur That's a good suggestion. Thanks. I'll write tests.

ptimson commented 2 years ago

Hey I have the same issue. I had ago at mimicking the regex without the negative lookbehind and came up with: ^_([\s\S]*?[^\\])_. Would need to write some unit tests as @sstur suggested. Hope this helps happy to contribute :)

yuxin1234 commented 2 years ago

@ptimson Thanks Peter. Please feel free to push your changes to this PR

ptimson commented 2 years ago

I dont think I can push to your repo I would have to make my own PR. I believe there a bunch of tests already for this I believe ^_([\s\S]*?[^\\])_ should work just a case of updating the regex and making sure they still pass.

yuxin1234 commented 2 years ago

@ptimson sounds good. Thanks.

luwangshell commented 2 years ago

Hi @sstur any chance you can review the PR? much appreciated.

yuxin1234 commented 2 years ago

@sstur could you please approve me running workflows? Thank you.

yuxin1234 commented 2 years ago

@sstur I have added unit test. Could you please review the PR? Thank you.

yuxin1234 commented 1 year ago

@sstur Simon, did you see my previous messages?

MSGInsurIt commented 1 year ago

Hi, @yuxin1234 in your tests you only check one possible scenario. you should make multiple scenarios, otherwise, this regex will break some valid scenarios.

Please take a look at this comment and the merge mentioned: https://github.com/sstur/draft-js-utils/issues/255#issuecomment-1357376778

yuxin1234 commented 1 year ago

@MSGInsurlt Thanks Joao. Will do.

yuxin1234 commented 1 year ago

@MSGInsurIt I added tests for 5 more different scenarios. All the tests passed locally.

MSGInsurIt commented 1 year ago

Hi @yuxin1234,

I just noticed something on your tests, are you really testing markdown? This "let markdown = 'BoldItalic';" does not seem to be markdown language...

Other than that, why not use tests added in the https://github.com/sstur/draft-js-utils/pull/254/files ???

And the look behind expression has been corrected in the safari browser I suppose that only the update on safari is needed

yuxin1234 commented 1 year ago

@MSGInsurIt Happy holidays! As you mentioned, since regex lookbehind has been fixed in WebKit, we just need wait for an updated Safari that supports negative lookbehind, so we probably don't need this PR anymore.

GeyseR commented 4 months ago

@yuxin1234 @MSGInsurIt I think we should re-cosider closing this PR and fix the issue. While the issue has been fixed a long time ago, occasional update of the minor version of the library (from 1.4.0 to 1.4.1) broke our app for many users who still use the older Safari versions.