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

Feature/spring cleaning #61

Merged
merged 23 commits into from
Apr 8, 2018
Merged

Feature/spring cleaning #61

merged 23 commits into from
Apr 8, 2018

Conversation

stuartsoft
Copy link
Contributor

@stuartsoft stuartsoft commented Feb 25, 2018

Minor updates to python script
Updated icon assets
Dark theme
Fix for dagger errors when using databinding (google/dagger#306)
Updated copyright and license

Copy link

@atoennis atoennis left a comment

Choose a reason for hiding this comment

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

I like the updates to the dependencies and adaptive launch icons 👍

There a few questions about some other modifications.

build.gradle Outdated
@@ -60,6 +60,11 @@ allprojects {
throw new GradleException("Wildcard dependency forbidden: ${requested.group}:${requested.name}:${requested.version}")
}
}
//Fix for showing 100+ errors from javac when using Android Databinding

Choose a reason for hiding this comment

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

I haven't noticed the errors referenced in that issue. However, it might be worth moving the comment on lines 52 and 53 to above line 57 for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't experienced the issue either, just noticed it while browsing on Dagger's github readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got those two comments moved to lines 55 & 56 👍

@@ -42,11 +42,6 @@ case "`uname`" in
;;
esac

# For Cygwin, ensure paths are in UNIX format before anything is touched.

Choose a reason for hiding this comment

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

Out of curiosity, what prompted the changes to gradle wrapper?

Copy link
Contributor Author

@stuartsoft stuartsoft Mar 15, 2018

Choose a reason for hiding this comment

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

I think I opened a completely fresh (File > New > New Project) Android Studio project and copied the new gradlew from that new project and pasted it in our starter project's gradlew

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But to answer your question specifically, not sure why things changed. I believe cywin is a windows platform thing.... FWIW

@@ -2,10 +2,8 @@
<resources>

<!-- Base application theme. -->
<style name="AppTheme" parent="Theme.AppCompat.Light.DarkActionBar">
<style name="AppTheme" parent="Theme.AppCompat.NoActionBar">

Choose a reason for hiding this comment

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

The dark theme looks good but curious why the change from the current theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like dark themes. That was literally the extent of my thinking :P
We can put the purple theme back in before accepting the PR if you want
or if you're ok with the dark theme, that's cool too. Whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we wanna use the dark theme: no changes to the PR required
If we wanna use the purple theme: I gotta undo some theme changes
If we wanna use both: That's cool too

@stuartsoft
Copy link
Contributor Author

@atoennis

@atoennis
Copy link

Here are my thoughts:

  1. Minor updates to python script - 👍
  2. Updated icon assets - 👍
  3. Dark theme - looks good but let's leave the theme as is
  4. Fix for dagger errors when using databinding (Dagger 2 swallows errors google/dagger#306) - Good to keep in mind but lets leave build.gradle as it was
  5. Updated copyright and license - I don't know much about the legalities of this but 👍

For 3 and 4 I'm thinking we leave leave them unchanged under the premise of fixing something only if it's an issue. Pre-emptively fixing something might be solving a problem that doesn't exist and more code = more complexity.

Thanks for keeping things current!

@stuartsoft
Copy link
Contributor Author

RE @atoennis
Dark theme is gone and purple is back
build.gradle is back to normal, but I did leave the target api at 27

@stuartsoft
Copy link
Contributor Author

@atoennis

@stuartsoft
Copy link
Contributor Author

@patrickhammond
Copy link

@stuartsoft After doing some quick research, please update the license year to 2014-2018. This reflects the year or original publish (per the commit history) through this year (where we're making changes).

Feel free to merge after that. Thanks!

@stuartsoft stuartsoft merged commit 3520f56 into atomicrobot:master Apr 8, 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