Skip to content
This repository was archived by the owner on Mar 24, 2025. It is now read-only.

Add phone log flutter plugin - #1

Closed
jiajiabingcheng wants to merge 5 commits into
google:masterfrom
jiajiabingcheng:master
Closed

Add phone log flutter plugin#1
jiajiabingcheng wants to merge 5 commits into
google:masterfrom
jiajiabingcheng:master

Conversation

@jiajiabingcheng

Copy link
Copy Markdown
Contributor

Hi team,

I just added a new plugin under the packages/...

Thanks,
Jiaming

@jiajiabingcheng

Copy link
Copy Markdown
Contributor Author

Hi @mehmetf, could please review this pr? Thanks!

@mehmetf
mehmetf self-requested a review June 20, 2018 21:41

@mehmetf mehmetf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just edited the .gitignore. I recommend syncing your client and creating a new PR. There's a lot of files in here that should not be part of the repo.

build/
ios/.generated/
packages
pubspec.lock

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need this file. I just updated the .gitignore file at the root. That should be enough.

@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2018 Jiaming Cheng

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe you need to set this to Google Inc. Not your personal name.

import 'package:phone_log/phone_log.dart';
```

### Methords

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix typo.

### Methords
```dart
/// Check phone log permission and return a [Future] with the result
static Future<bool> checkPermission(Permission permission);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid static methods. This makes the plugin files non-mockable.

@@ -0,0 +1,13 @@
.DS_Store

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove.

<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.example.phonelogexample">

<!-- The INTERNET permission is required for development. Specifically,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove comments.

<layer-list xmlns:android="http://schemas.android.com/apk/res/android">
<item android:drawable="@android:color/white" />

<!-- You can insert your own image assets here -->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove comments.

@@ -0,0 +1,6 @@
#Fri Jun 23 08:50:38 CEST 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You only need this file. Revert everything else in this directorly. The updated .gitignore should handle all this.

@@ -0,0 +1,44 @@
.idea/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revert.

@@ -0,0 +1,36 @@
.idea/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revert.

@jiajiabingcheng

Copy link
Copy Markdown
Contributor Author

Thanks a lot Mehmet. As you suggested I'm closing this one and pulling a new one with addressing all the comments.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants