Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Better documentation of Android Platform Views modes #4187

Merged

Conversation

ludwiktrammer
Copy link
Contributor

@ludwiktrammer ludwiktrammer commented Jul 23, 2021

Fixes flutter/flutter#85215

Documentation improvement - documents the difference between the two modes more clearly, including the SDK requirement for the Virtual displays mode (which was previously missing, as described in flutter/flutter#85215)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Jul 23, 2021
@github-actions github-actions bot added the p: webview_flutter Edits files for a webview_flutter plugin label Jul 23, 2021
Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request. The text looks good, I just made some suggestions to fix some spelling mistakes.

packages/webview_flutter/webview_flutter/README.md Outdated Show resolved Hide resolved
packages/webview_flutter/webview_flutter/README.md Outdated Show resolved Hide resolved
Comment on lines +53 to +59
```groovy
android {
defaultConfig {
minSdkVersion 19
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove redundant spaces to align code block the same way as the same block mentioned in the "Using Virtual displays" section.

Suggested change
```groovy
android {
defaultConfig {
minSdkVersion 19
}
}
```
```groovy
android {
defaultConfig {
minSdkVersion 19
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, but they are not redundant - this is inside an enumerated list and the indentation is needed so that the code block remains a part of the correct list item.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but then maybe also make sure the indentation is also used in the "Using Virtual displays" section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no list in the "Using Virtual displays" section (since it would be a one-element list)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, isn't it the exact same android/app/build.gradle file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, isn't it the exact same android/app/build.gradle file?

Nevermind, I understand now. It is part of the list in the README.md, sorry for the confusion.

ludwiktrammer and others added 2 commits July 28, 2021 11:19
Co-authored-by: Maurits van Beusekom <maurits@baseflow.com>
Co-authored-by: Maurits van Beusekom <maurits@baseflow.com>
Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

Looks great, just one more thing. Could you add a line to the CHANGELOG.md describing the change using the "NEXT" version? Something like this:

NEXT:

* Improved the documentation on using the different Android Platform View modes.

Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

@ludwiktrammer sorry for being such a pain, but the you should rebase with master branch as you same to have a merge conflict on the CHANGELOG.md file now.

@ludwiktrammer
Copy link
Contributor Author

@ludwiktrammer Sure. As to the changelog file: should I put literal "NEXT" or the next number ("2.0.12")?

@mvanbeusekom
Copy link
Contributor

@ludwiktrammer Sure. As to the changelog file: should I put literal "NEXT" or the next number ("2.0.12")?

You can put the literal "NEXT" as it is not necessary to release a new version just for the documentation change. So we will include it with the next version. The "NEXT" literal is a reminder for the next change to include this in the release notes.

@ludwiktrammer
Copy link
Contributor Author

@mvanbeusekom Ok, thanks. Then it's done :)

Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much for this contribution.

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jul 28, 2021
@fluttergithubbot fluttergithubbot merged commit f506daa into flutter:master Jul 28, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2021
@stuartmorgan
Copy link
Contributor

You can put the literal "NEXT" as it is not necessary to release a new version just for the documentation change.

For future reference, this is not our policy. See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#faq

Do I need to update the version if I'm just changing the README? Yes. Most people read the README on pub.dev, not GitHub, so a README change is not very useful until it's published.

In this case it's not a big deal since we expect to release webview_flutter again soon and can get it then, but README fixes shouldn't rely on that any more than code fixes.

@mvanbeusekom
Copy link
Contributor

Apologies, my bad. Thank you for pointing that out @stuartmorgan.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: webview_flutter Edits files for a webview_flutter plugin waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
4 participants