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

Add AM/PM circles #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add AM/PM circles #2

wants to merge 4 commits into from

Conversation

NeoLSN
Copy link

@NeoLSN NeoLSN commented Jun 22, 2016

No description provided.

@goncalossilva
Copy link
Member

First of all, thanks for the PR!

Unfortunately, this library tracks a Google library, just making the necessary changes to make it usable in a library form. For this reason, I'm inclined to not accept this, though you are welcome to maintain a fork!

@NeoLSN
Copy link
Author

NeoLSN commented Jun 22, 2016

https://android.googlesource.com/platform/frameworks/opt/datetimepicker/+/f41bcf49a4ab0b2f204533c757069cf2edfabc97/src/com/android/datetimepicker/time/AmPmCirclesView.java
There is the feature in the library, but I just can not find in here. This is the reason why I added this. Is any chance will you use another way let this feature back?

@goncalossilva
Copy link
Member

My bad! Will review it right away.

compile 'com.android.support:support-v4:23.3.0'
compile 'com.android.support:appcompat-v7:23.3.0'
compile 'com.android.support:support-v4:24.0.0@aar'
compile 'com.android.support:appcompat-v7:24.0.0@aar'
Copy link
Member

Choose a reason for hiding this comment

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

Why @aar on both of these? Shouldn't be needed?

Copy link
Author

@NeoLSN NeoLSN Jun 23, 2016

Choose a reason for hiding this comment

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

For lean dependencies.
You can run gradle task - [androidDependencies] and see the results. For use there is no different.

No @aar

Information:Gradle tasks [androidDependencies]
:DateTimePickerLibrary:androidDependencies
debug
+--- com.android.support:support-v4:24.0.0
|    \--- LOCAL: internal_impl-24.0.0.jar
\--- com.android.support:appcompat-v7:24.0.0
     +--- com.android.support:support-v4:24.0.0
     |    \--- LOCAL: internal_impl-24.0.0.jar
     +--- com.android.support:support-vector-drawable:24.0.0
     |    \--- com.android.support:support-v4:24.0.0
     |         \--- LOCAL: internal_impl-24.0.0.jar
     \--- com.android.support:animated-vector-drawable:24.0.0
          \--- com.android.support:support-vector-drawable:24.0.0
               \--- com.android.support:support-v4:24.0.0
                    \--- LOCAL: internal_impl-24.0.0.jar

With @aar

Information:Gradle tasks [androidDependencies]
:DateTimePickerLibrary:androidDependencies
debug
+--- com.android.support:support-v4:24.0.0
|    \--- LOCAL: internal_impl-24.0.0.jar
\--- com.android.support:appcompat-v7:24.0.0

@goncalossilva
Copy link
Member

goncalossilva commented Jun 22, 2016

Could you improve the code parity with the original source? This PR uses different names, different code locations, etc, which will make it much harder to update in the future.

@NeoLSN
Copy link
Author

NeoLSN commented Jun 23, 2016

Can you give me some suggestions about how to improve? Because I don't know what code style you prefer, or can you just ignore my PR use other way to do that?

@goncalossilva
Copy link
Member

Sorry for the delay getting back to you. Workload has been increasing lately and it's been a bit hard to keep up.

What I mean about code parity I'm not talking about my code style preference... just to try to have 1-on-1 parity of the code structure, naming and flow with the original source by Google.

I can look more deeply into this during August, but I'm hoping my feedback is clearer this time around!

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.

None yet

2 participants