Skip to content

Conversation

@bwikbs
Copy link
Member

@bwikbs bwikbs commented Oct 7, 2021

Signed-off-by: MuHong Byun mh.byun@samsung.com

@github-actions github-actions bot added needs-publishing The package should be published after merge p: webview_flutter labels Oct 7, 2021
@bwikbs
Copy link
Member Author

bwikbs commented Oct 7, 2021

@HakkyuKim @swift-kim @bbrto21
I guess.. It seems necessary to upload the flutter version to the analyze task. When should we do it?

@HakkyuKim
Copy link
Contributor

HakkyuKim commented Oct 7, 2021

It seems that flutter analyze became similar to dart analyze after bumping to 2.5.1. The warnings look similar to #223.

@HakkyuKim
Copy link
Contributor

I can resolve most errors/warnings in #234 except for "Document all public members" for url_launcher and webview_flutter as I'm not the writer of those packages.

@bwikbs
Copy link
Member Author

bwikbs commented Oct 7, 2021

@HakkyuKim 👌 I'll do that stuff!

@swift-kim
Copy link
Member

👌 I'll do that stuff!

@bwikbs Could you make it as a separate PR? This PR is supposed to only change webview_flutter-related things.

@bwikbs
Copy link
Member Author

bwikbs commented Oct 7, 2021

@bwikbs Could you make it as a separate PR? This PR is supposed to only change webview_flutter-related things.

Sure~. It's was just for testing purposes. Don't worry 😄
(I just wanted to know how much task was remained.)

@HakkyuKim
Copy link
Contributor

@bwikbs
I'll add // ignore_for_file: public_member_api_docs for url_launcher to #234. Then we can first merge #234 tomorrow after I fix the server for testing package_info_plus. You can put webview_flutter changes here after rebasing it to master. Is this OK?

@bwikbs
Copy link
Member Author

bwikbs commented Oct 7, 2021

@HakkyuKim 🙆‍♂️

@bwikbs
Copy link
Member Author

bwikbs commented Oct 8, 2021

@HakkyuKim I have simple question!
When do we apply '2.5.1'?

#234 -> apply '2.5.1' -> #236 OR #234 -> #236 -> apply '2.5.1' ?
Either way ci would fail in step 2? Which one is better?

@HakkyuKim
Copy link
Contributor

@bwikbs
You can apply '2.5.1' to the CI script in this PR as done in #216 since it's necessary for this package update.
So #234 -> #236 (which includes updating the CI script to '2.5.1').

@bwikbs
Copy link
Member Author

bwikbs commented Oct 8, 2021

@HakkyuKim
Got it! thanks! 😄

@swift-kim
Copy link
Member

Beware that updating the flutter-version in https://github.com/flutter-tizen/plugins/blob/master/.github/workflows/analyze.yml may break things since Dart 2.12 and 2.14 use different formatting for cascades. Thus it would be better to make it as a separate PR.

@HakkyuKim
Copy link
Contributor

@swift-kim @bwikbs
In that case, I'll update flutter-version of the CI script first in a separate PR.

Signed-off-by: MuHong Byun <mh.byun@samsung.com>
@bwikbs bwikbs marked this pull request as ready for review October 8, 2021 07:12
@bwikbs
Copy link
Member Author

bwikbs commented Oct 8, 2021

After a short discussion, we decided to include #240 to this PR.

Copy link
Contributor

@HakkyuKim HakkyuKim left a comment

Choose a reason for hiding this comment

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

Could you also update to

dependencies:
  webview_flutter_tizen: ^0.3.7

in packages/webview_flutter/README.md?

Signed-off-by: MuHong Byun <mh.byun@samsung.com>
@bwikbs
Copy link
Member Author

bwikbs commented Oct 12, 2021

Could you also update to

dependencies:
  webview_flutter_tizen: ^0.3.7

in packages/webview_flutter/README.md?

!!! Good point! 👍

Copy link
Contributor

@HakkyuKim HakkyuKim left a comment

Choose a reason for hiding this comment

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

LGTM

flutter:
sdk: flutter
webview_flutter: ^2.0.4
webview_flutter: ^2.1.1
Copy link
Member

Choose a reason for hiding this comment

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

I don't care but please be aware of the implicit indirect dependency (webview_flutterwebview_flutter_platform_interface).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry 😢 I can't get your point. What do you concern about?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you explicitly add a dependency about webview_flutter_platform_interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

webview_flutter_platform_interface is dependent webview_flutter. Do I have to use it unconditionally to use a higher version of webview_flutter?
What's the point of being explicit? (I'm really curious to ask... )

Copy link
Member

Choose a reason for hiding this comment

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

The webview_flutter_tizen package is an implementation of webview_flutter_platform_interface, so it's very natural to specify which version of webview_flutter_platform_interface the package implements.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right! I'll fix it!

Signed-off-by: MuHong Byun <mh.byun@samsung.com>
@bwikbs bwikbs merged commit 8a21e27 into flutter-tizen:master Oct 14, 2021
@swift-kim
Copy link
Member

@bwikbs The person who merged a PR marked as "needs-publishing" is responsible for publishing the updated package. Please ask @bbrto21 if you don't know how to.

@bwikbs
Copy link
Member Author

bwikbs commented Oct 14, 2021

@swift-kim I've already did that. Thanks for letting me know anyway. 😄

@HakkyuKim HakkyuKim mentioned this pull request Oct 19, 2021
59 tasks
bwikbs added a commit to bwikbs/plugins that referenced this pull request Mar 26, 2022
…zen#236)

* [webview_flutter] Apply upstream (webview_flutter-v2.1.1)

Signed-off-by: MuHong Byun <mh.byun@samsung.com>

* Add reviewer's comment

Signed-off-by: MuHong Byun <mh.byun@samsung.com>

* Add reviewer's comment

Signed-off-by: MuHong Byun <mh.byun@samsung.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-publishing The package should be published after merge p: webview_flutter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants