search

Sub6Resources / flutter_html

A Flutter widget for rendering static html as Flutter widgets (Will render over 80 different html tags!)
https://pub.dev/packages/flutter_html
MIT License
1.76k stars 815 forks source link

Nullsafety #548

Closed tneotia closed 3 years ago

tneotia commented 3 years ago

Refactor to nullsafety. Taking this in stages, so I've marked it as a draft. If you'd like, you can look over what I've done so far and let me know if anything should be changed.

Update: Migration complete - I've tested with as many HTML edge cases I can think of and it is still working great. I used the ! operator more than I anticipated, but I don't know if that is an issue, per se.

Fixes #511, fixes #468, fixes #399, fixes #568, fixes #274 (as far as I can tell)

erickok commented 3 years ago

I will go over this fully some time if you want. I certianly see some cases where I think we should use less !s.

tneotia commented 3 years ago

Yes, please take a look and add commits if you feel necessary that would be great!. Imo this should be merged before anything else to prevent more merge conflicts, since you can't just run dart migrate again after already completing a migration - you have to completely discard the nullsafety changes and start again as far as I can tell.

tneotia commented 3 years ago

@erickok I've fixed all the merge conflicts. I think this is good to go apart from one thing I was concerned about, which I created a review for above.

This was indeed an issue and I fixed it with the most recent commit.

jlubeck commented 3 years ago

@tneotia sounds like you can update the chewie_audio dependency now? https://github.com/Sub6Resources/chewie_audio/pull/12#issuecomment-791747859

erickok commented 3 years ago

Yes @jlubeck but please allow is to do some proper testing. I know we all want a nullability-supporting flutter_html but this is a big job and we also don't want to have to release a dozen patches later.

jlubeck commented 3 years ago

Yes @jlubeck but please allow is to do some proper testing. I know we all want a nullability-supporting flutter_html but this is a big job and we also don't want to have to release a dozen patches later.

For sure! Sorry if my comment was disrespectful, wasn't aimed at rushing anything. Just super excited to get null safety flutter_html. Sorry again!

tneotia commented 3 years ago

@jlubeck done. Edit: it you'd like please test this as well. You use the package for Web extensively and as a result I think you can provide another perspective when testing.

@erickok please do whatever testing is necessary, this really needs to squash all the existing bugs and not create any new regressions.

Chewie audio still works great:

image

erickok commented 3 years ago

I am going to merge this and release a preview version.

roskimlong commented 3 years ago

please public a new version for iframe video, I'm waiting for that. @erickok

erickok commented 3 years ago

@roskimlong I'm not sure what you mean with iframe video?

roskimlong commented 3 years ago

I tried to added iframe YouTube video not work but for link src beside YouTube is worked well

tneotia commented 3 years ago

I tried to added iframe YouTube video not work but for link src beside YouTube is worked well

Youtube iframe should work as I have used it in my app. Can you give us an example of a URL that doesn't work, preferably creating a new issue? Thanks.

erickok commented 3 years ago

@roskimlong see #513 for some tips. But please make a new issue, with example code.

roskimlong commented 3 years ago

@erickok i want to tried first before I create new issue brother ❤️❤️ I tried to fix as I can and then I will add new issues brother

roskimlong commented 3 years ago

Anyway thanks for advice brothers, @tneotia @erickok ❤️