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

migrated to androidx #10

Closed
wants to merge 5 commits into from
Closed

migrated to androidx #10

wants to merge 5 commits into from

Conversation

kibotu
Copy link

@kibotu kibotu commented Sep 21, 2018

No description provided.

Copy link

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks like there's a few things going on here, so let's try to keep it a pure migration and keep functional changes to a separate discussion. Also please abide by the existing code style (formatting, nullability annotations)

@@ -1,5 +1,9 @@
# Changelog

## 4.0.0 (21.8.2018)

Choose a reason for hiding this comment

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

Leave this out for now, maintainers will update the changelog when a new release is made

compile 'com.android.support:appcompat-v7:24.0.0'
implementation project(':calligraphy')
// implementation 'io.github.inflationx:viewpump:1.0.0'
implementation 'com.github.kibotu:ViewPump:v2.0.0'

Choose a reason for hiding this comment

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

What's this?

@@ -37,14 +36,16 @@ public void run() {
}, 1000);

handler.postDelayed(new Runnable() {
@Override public void run() {

Choose a reason for hiding this comment

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

Nit - don't change the style here

@@ -23,19 +25,19 @@ public static Fragment getInstance() {
}

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle state) {
public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container, Bundle state) {

Choose a reason for hiding this comment

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

This project assumes nonnull is the default, please only use nullable annotations as needed


/**
* Created by chris on 17/03/15.
* For Calligraphy.
*/
public class TextField extends TextView {
public class TextField extends AppCompatTextView {

Choose a reason for hiding this comment

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

What's this change for?

jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.2.1'
classpath 'com.android.tools.build:gradle:3.3.0-alpha11'
classpath 'com.github.dcendents:android-maven-gradle-plugin:2.1'

Choose a reason for hiding this comment

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

What's this?

jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.2.1'
classpath 'com.android.tools.build:gradle:3.3.0-alpha11'

Choose a reason for hiding this comment

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

Please use the latest stable

ext {
isReleaseVersion = has("release")
versionCodeInt = getProperty('VERSION_CODE').toInteger()
}
}

task wrapper(type: Wrapper) {

Choose a reason for hiding this comment

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

Why remove this vs update it?

}
}

allprojects {
repositories {
maven { url 'https://jitpack.io' }

Choose a reason for hiding this comment

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

What's this for?

}

apply from: rootProject.file('gradle/deploy.gradle')
//apply from: rootProject.file('gradle/deploy.gradle')

Choose a reason for hiding this comment

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

Why is this commented?

Copy link

@jbarr21 jbarr21 left a comment

Choose a reason for hiding this comment

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

please address remaining feedback

@jbarr21
Copy link

jbarr21 commented Nov 14, 2018

Thanks for your contribution! I've made the changes for the comments that were requested and have merged it manually into master.

@jbarr21 jbarr21 closed this Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants