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

[in_app_purchase] improve readme #3731

Merged
merged 21 commits into from
Apr 6, 2021
Merged

[in_app_purchase] improve readme #3731

merged 21 commits into from
Apr 6, 2021

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Mar 18, 2021 •
edited
Loading

This improves the readme of the plugin.

  1. Added missing feature in readme
  2. Added the preparation section header
  3. Link users to contribution guide
  4. Add more code samples
  5. Fix some typos

Upcoming in a separate PR: video demos for the plugin.

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 Flutter Style Guide and the C++, Objective-C, Java style guides.
  • 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.

@cyanglaz cyanglaz requested a review from LHLL as a code owner March 18, 2021 18:35
@google-cla google-cla bot added the cla: yes label Mar 18, 2021
@cyanglaz cyanglaz requested a review from jayoung-lee March 18, 2021 18:37
jayoung-lee
jayoung-lee reviewed Mar 18, 2021
View reviewed changes
jayoung-lee
jayoung-lee reviewed Mar 18, 2021
View reviewed changes
@jayoung-lee jayoung-lee requested a review from kwalrath March 19, 2021 01:24
jayoung-lee
jayoung-lee reviewed Mar 19, 2021
View reviewed changes
@kwalrath
Copy link

Before I can do a detailed review of this README, I need some info:

  • What should the main sections be?
    • It kind of seems like Preparation should be a section before Getting Started, maybe named API overview.
    • Should we rename Getting Started to something like Example code? (Btw, I love that you have so much example code!)
  • The purpose of the Development section is unclear.
    • Who is the first paragraph aimed at? A user of the API or a contributor to the package?
    • The second paragraph (about this being a beta plugin) seems pretty important. Should it be up at the top of the README?
  • We need to establish some terminology. (It's fine to mention all the terms people might expect, but after the main term is established we should stick with it.) Which term do you want to use from the following groups?
    • in-app purchase or IAP?
    • storefront, underlying shop, or underlying store?

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Mar 22, 2021 •
edited
Loading

@kwalrath Here are my answers to the questions :)

  • What should the main sections be?

    • It kind of seems like Preparation should be a section before Getting Started, maybe named API overview.

We don't have a Preparation section in the main README. It's only in the example app README, the purpose is to teach users how to setup and run the example app.

  • Should we rename Getting Started to something like Example code? (Btw, I love that you have so much example code!)

Getting Started is more than just Example code, due to the speciality of the plugin, there are a lot of initial setup need to be done in the GooglePlay console and App Store Connect before the code can work.

  • The purpose of the Development section is unclear.

    • Who is the first paragraph aimed at? A user of the API or a contributor to the package?

I renamed the Development to Contributing to this plugin so the purpose might be a little more clear? It is for contributors.

  • The second paragraph (about this being a beta plugin) seems pretty important. Should it be up at the top of the README?

I think you are right. It can be a second paragraph in the section # In App Purchase, right before ## Features. WDTY?

  • We need to establish some terminology. (It's fine to mention all the terms people might expect, but after the main term is established we should stick with it.) Which term do you want to use from the following groups?

    • in-app purchase or IAP?

in-app purchase

  • storefront, underlying shop, or underlying store?

underlying store .

@cyanglaz
Copy link
Contributor Author

@kwalrath friendly ping regarding my last response :)

@kwalrath
Copy link

The gifs seem really large — both in number of bytes and in visual size. @jayoung-lee & @filiph, what do you think a good size would be?

cyanglaz
cyanglaz commented Mar 29, 2021
View reviewed changes
@cyanglaz
Copy link
Contributor Author

The gifs seem really large — both in number of bytes and in visual size. @jayoung-lee & @filiph, what do you think a good size would be?

I manually set the height to be 400 in code, it looks ok to me, if you guys like it, I can update the gif file so it is smaller by default.

@kwalrath
Copy link

The gifs seem really large — both in number of bytes and in visual size. @jayoung-lee & @filiph, what do you think a good size would be?

I manually set the height to be 400 in code, it looks ok to me, if you guys like it, I can update the gif file so it is smaller by default.

400 px high lgtm

@kwalrath
Copy link

I'm working on a new version of the top README and have a couple of questions:

  1. Should the API links be to API docs (e.g. in_app_purchase, billing_client_wrappers, and store_kit_wrappers) instead of to .dart files?
  2. The README refers to PlayStore, but I don't see that anywhere in the API.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Mar 29, 2021 •
edited
Loading

I'm working on a new version of the top README and have a couple of questions:

  1. Should the API links be to API docs (e.g. in_app_purchase, billing_client_wrappers, and store_kit_wrappers) instead of to .dart files?

links to API docs sounds good to me.

  1. The README refers to PlayStore, but I don't see that anywhere in the API.

PlayStore is the PlayStore on android phones: https://play.google.com/store

@kwalrath
Copy link

PlayStore is the PlayStore on android phones: https://play.google.com/store

PlayStore is code fonted like it's an API, but I don't see it in the source code anywhere. I do see IAPSource.GooglePlay in the source code, but since IAPSource isn't shown in the code, it doesn't seem necessary to refer to the GooglePlay (or AppStore) identifier in the text. I'll just change those to the store names.

@kwalrath
Copy edit README.md
0af8309 
I made a bunch of edits, some of which are particular to this README, and some of which are more generally applicable. Here are the more interesting ones that might apply to all package READMEs:

* DON'T have a title, because I think it looks better on pub.dev to not have a title (e.g. compare https://pub.dev/packages/yaml with https://pub.dev/packages/crypto).
* DO start with a one-line description, followed (if the package has a dedicated repo) by badges.
* DO use hand-created TOCs if necessary for longer pages/sections.
* DO mention terms that people might use to search for your package (but after that, be consistent about which terms you use).

Also, some other changes that aren't particular to READMEs:

* Keep line length < 80 chars.
* Format code correctly.
* Supply alt text for images.
* Be succinct. Don't say please. Use consistent terminology. And other things that are [common recommendations for developer docs](https://developers.google.com/style/highlights).

There's a PENDING note where I happened to notice some code that seemed to be duplicated; we need to fix that before committing this.

I tested the page in a github-style markdown previewer, and also in the GitHub Flavored Markdown mode of https://dart-lang.github.io/markdown (which is what I think/hope pub.dev uses).

Please let me know what you think.
@kwalrath kwalrath requested a review from filiph March 30, 2021 01:36
@cyanglaz
Copy link
Contributor Author

@kwalrath Yeah, we shouldn't code font the "PlayStore".

@kwalrath
Copy link

I think we just need to fix the _listenToPurchaseUpdated/_handlePurchaseUpdates code and remove that PENDING note, and then this PR is good to go!

@cyanglaz
Copy link
Contributor Author

_listenToPurchaseUpdated

Done!

@cyanglaz cyanglaz requested a review from jayoung-lee March 30, 2021 17:58
@cyanglaz
Copy link
Contributor Author

@kwalrath @jayoung-lee Thanks for helping out, this looks pretty good now, could you please take a final review?

@kwalrath
Copy link

@cyanglaz it looks like the "[PENDING: We already showed code like this above (except the handler was called _listenToPurchaseUpdated). Copy this up there and remove from here?]" text is still there, and it hasn't been completely addressed. The code is still in two places, with different handlers (e.g. onDone & onError are only in the second example), so it's a bit confusing what exactly your code should look like.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 2, 2021

@kwalrath Sorry I missed that part. I copied the second one to the top. Fixed!

@cyanglaz cyanglaz mentioned this pull request Apr 2, 2021
Closed
11 tasks
kwalrath
kwalrath approved these changes Apr 5, 2021
View reviewed changes
Copy link

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

lgtm once you fix the order of the usage toc and change that numbered list to a bulleted one (since it isn't a sequence: https://developers.google.com/style/highlights#formatting,-punctuation,-and-organization). Can't believe I didn't notice that before!

@cyanglaz cyanglaz 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 Apr 6, 2021
@fluttergithubbot fluttergithubbot merged commit 4bd178b into flutter:master Apr 6, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 6, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 6, 2021
@engine-flutter-autoroll
4bd178b [in_app_purchase] improve readme (flutter/plugins#3731)
88f56d2
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@fotiDim
[in_app_purchase] improve readme (flutter#3731)
7163347
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@LHLL LHLL Awaiting requested review from LHLL

@filiph filiph Awaiting requested review from filiph

@jayoung-lee jayoung-lee Awaiting requested review from jayoung-lee

1 more reviewer

@kwalrath kwalrath kwalrath approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla: yes p: in_app_purchase 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cyanglaz @kwalrath @jayoung-lee @stuartmorgan-g @fluttergithubbot