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

Nullsafety #548

Merged
merged 13 commits into from
Mar 6, 2021
Merged

Nullsafety #548

merged 13 commits into from
Mar 6, 2021

Conversation

tneotia
Copy link
Collaborator

@tneotia tneotia commented Feb 17, 2021

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)

@tneotia tneotia marked this pull request as draft February 17, 2021 21:41
@tneotia tneotia marked this pull request as ready for review February 17, 2021 23:16
@tneotia tneotia mentioned this pull request Feb 17, 2021
@erickok
Copy link
Collaborator

erickok commented Feb 24, 2021

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
Copy link
Collaborator Author

tneotia commented Feb 24, 2021

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
Copy link
Collaborator Author

tneotia commented Mar 4, 2021

@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.

Copy link
Collaborator Author

@tneotia tneotia left a comment

Choose a reason for hiding this comment

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

This can be ignored.

@jlubeck
Copy link

jlubeck commented Mar 5, 2021

@tneotia sounds like you can update the chewie_audio dependency now? Sub6Resources/chewie_audio#12 (comment)

@erickok
Copy link
Collaborator

erickok commented Mar 5, 2021

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
Copy link

jlubeck commented Mar 5, 2021

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
Copy link
Collaborator Author

tneotia commented Mar 5, 2021

@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

Copy link
Collaborator

@erickok erickok left a comment

Choose a reason for hiding this comment

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

Great job. Almost all my comment are very easy to fix (or not even that needs to be fixed).

pubspec.yaml Outdated Show resolved Hide resolved
pubspec.yaml Show resolved Hide resolved
lib/src/widgets/iframe_web.dart Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
example/pubspec.yaml Outdated Show resolved Hide resolved
lib/src/replaced_element.dart Show resolved Hide resolved
lib/src/replaced_element.dart Show resolved Hide resolved
lib/src/styled_element.dart Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
lib/image_render.dart Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@tneotia tneotia left a comment

Choose a reason for hiding this comment

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

This can be ignored.

README.md Show resolved Hide resolved
@erickok
Copy link
Collaborator

erickok commented Mar 6, 2021

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

@erickok erickok merged commit e421c47 into Sub6Resources:master Mar 6, 2021
@roskimlong
Copy link

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

@erickok
Copy link
Collaborator

erickok commented Mar 28, 2021

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

@roskimlong
Copy link

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

@tneotia
Copy link
Collaborator Author

tneotia commented Mar 28, 2021

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
Copy link
Collaborator

erickok commented Mar 28, 2021

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

@roskimlong
Copy link

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants