-
Notifications
You must be signed in to change notification settings - Fork 3
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 resources module with themes/styles #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much man! I literally thought that adding a theme would be like 1-2 files.
The playground is above and beyond ✊ I'll leave for @hitherejoe to take a look tomorrow and then maybe we can merge, and I'll raise my PR that integrates with this new theme for you to peek at?
|
||
import androidx.appcompat.app.AppCompatActivity | ||
|
||
class ThemePlaygroundActivity : AppCompatActivity(R.layout.activity_theme_playground) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation libraries.androidxAppCompat | ||
implementation libraries.androidxConstraintLayout | ||
implementation libraries.kotlinStdLib | ||
implementation libraries.materialComponents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
class MainActivity : AppCompatActivity() { | ||
class MainActivity : AppCompatActivity(R.layout.activity_main) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/build.gradle
Outdated
@@ -21,6 +21,8 @@ android { | |||
dependencies { | |||
implementation project(':core') | |||
implementation project(':feed') | |||
implementation project(':navigation') | |||
implementation project(':resources') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 we pulled out the feed
stuff into a library module because we didn't want logic in app
.
I think pulling in navigation
and resources
are fine to support the "meta" ThemePlaygroundActivity
|
||
<application> | ||
|
||
<activity android:name=".ThemePlaygroundActivity" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a real life app, I guess this Manifest would exist in both the main and debug sourcesets so we only had ThemePlaygroundActivity in debug
, but the meta-data
block would be alright here since the theme needs it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I think we could get away with it in debug, theoretically, unless I'm missing something!
android:height="24dp" | ||
android:viewportWidth="24.0" | ||
android:viewportHeight="24.0" | ||
android:tint="?attr/colorOnPrimarySurface"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:paddingBottom="16dp" | ||
android:clipToPadding="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed for something?
Do you pull copy these activity theme playground from somewhere?
Is it something that could exist as a library that could be added to any project? Where it would take the correct theme from the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the layout for ThemePlaygroundActivity
.
Yep, I copied it from my own playground layout in a MDC-Android sample code repo:
https://github.com/ricknout/android-mdc-theming/blob/master/app/src/main/res/layout/activity_theming.xml
It could definitely form part of a library in future 👍 agree it does bloat the PR a bit for now though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed for something?
I meant the clip to padding false 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol, so the components on the playground screen can scroll past the bottom padding without being clipped
<?xml version="1.0" encoding="utf-8"?> | ||
<resources> | ||
|
||
<!-- Theme colors --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I need to upgrade to Q now to test this out 🆗 👌
MIIEqDCCA5CgAwIBAgIJANWFuGx90071MA0GCSqGSIb3DQEBBAUAMIGUMQswCQYDVQQGEwJVUzETMBEGA1UECBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNTW91bnRhaW4gVmlldzEQMA4GA1UEChMHQW5kcm9pZDEQMA4GA1UECxMHQW5kcm9pZDEQMA4GA1UEAxMHQW5kcm9pZDEiMCAGCSqGSIb3DQEJARYTYW5kcm9pZEBhbmRyb2lkLmNvbTAeFw0wODA0MTUyMzM2NTZaFw0zNTA5MDEyMzM2NTZaMIGUMQswCQYDVQQGEwJVUzETMBEGA1UECBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNTW91bnRhaW4gVmlldzEQMA4GA1UEChMHQW5kcm9pZDEQMA4GA1UECxMHQW5kcm9pZDEQMA4GA1UEAxMHQW5kcm9pZDEiMCAGCSqGSIb3DQEJARYTYW5kcm9pZEBhbmRyb2lkLmNvbTCCASAwDQYJKoZIhvcNAQEBBQADggENADCCAQgCggEBANbOLggKv+IxTdGNs8/TGFy0PTP6DHThvbbR24kT9ixcOd9W+EaBPWW+wPPKQmsHxajtWjmQwWfna8mZuSeJS48LIgAZlKkpFeVyxW0qMBujb8X8ETrWy550NaFtI6t9+u7hZeTfHwqNvacKhp1RbE6dBRGWynwMVX8XW8N1+UjFaq6GCJukT4qmpN2afb8sCjUigq0GuMwYXrFVee74bQgLHWGJwPmvmLHC69EH6kWr22ijx4OKXlSIx2xT1AsSHee70w5iDBiK4aph27yH3TxkXy9V89TDdexAcKk/cVHYNnDBapcavl7y0RiQ4biu8ymM8Ga/nmzhRKya6G0cGw8CAQOjgfwwgfkwHQYDVR0OBBYEFI0cxb6VTEM8YYY6FbBMvAPyT+CyMIHJBgNVHSMEgcEwgb6AFI0cxb6VTEM8YYY6FbBMvAPyT+CyoYGapIGXMIGUMQswCQYDVQQGEwJVUzETMBEGA1UECBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNTW91bnRhaW4gVmlldzEQMA4GA1UEChMHQW5kcm9pZDEQMA4GA1UECxMHQW5kcm9pZDEQMA4GA1UEAxMHQW5kcm9pZDEiMCAGCSqGSIb3DQEJARYTYW5kcm9pZEBhbmRyb2lkLmNvbYIJANWFuGx90071MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEEBQADggEBABnTDPEF+3iSP0wNfdIjIz1AlnrPzgAIHVvXxunW7SBrDhEglQZBbKJEk5kT0mtKoOD1JMrSu1xuTKEBahWRbqHsXclaXjoBADb0kkjVEJu/Lh5hgYZnOjvlba8Ld7HCKePCVePoTJBdI4fvugnL8TsgK05aIskyY0hKI9L8KfqfGTl1lzOv2KoWD0KWwtAWPoGChZxmQ+nBli+gwYMzM1vAkP+aayLe0a1EQimlOalO762r0GXO0ks+UeXde2Z4e+8S/pf7pITEI/tP+MxJTALw9QUWEv9lKTk+jkbqxbsh8nfBUapfKqYn0eidpwq2AzVp3juYl7//fKnaPhJD9gs= | ||
</item> | ||
</string-array> | ||
<string-array name="com_google_android_gms_fonts_certs_prod"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these generated from? Is there any issue with these being in the open? (I guess since they'll need to be in the APK, they're not secret secret).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are auto-generated when going through the Android Studio downloadable font wizard. I think it's fine, I've done something similar in some of my own repos. Alternatively we could package the font files and use local XML fonts, but that would have an impact on APK size and maybe the complexity of this PR. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me, as long as it's not like tied to your account and cause you problems or anything
🤔 In but it works at runtime, and compiles fine. If we change it to I think it ought to be |
(I merged this into my branch, if this is good, please merge commit, don't squash 😅 I really don't want to deal with merge conflicts/rebasing 😂 ) |
🤔 I've only seen it on scrolling containers, I didn't think it'd work on
the constraint layout.
…On Mon, 12 Aug 2019, 21:20 Nick Rout, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In resources/src/main/res/layout/activity_theme_playground.xml
<#27 (comment)>:
> @@ -0,0 +1,243 @@
+<?xml version="1.0" encoding="utf-8"?>
+<ScrollView
+ xmlns:android="http://schemas.android.com/apk/res/android"
+ xmlns:tools="http://schemas.android.com/tools"
+ xmlns:app="http://schemas.android.com/apk/res-auto"
+ android:layout_width="match_parent"
+ android:layout_height="match_parent"
+ tools:context=".ThemePlaygroundActivity">
+
+ <androidx.constraintlayout.widget.ConstraintLayout
+ android:layout_width="match_parent"
+ android:layout_height="wrap_content"
+ android:paddingBottom="16dp"
+ android:clipToPadding="false">
Oh lol, so the components on the playground screen can scroll past the
padding without being clipped
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27?email_source=notifications&email_token=AAUN6G276HAYTAE4F72SDMLQEHAYLA5CNFSM4IK4KPY2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJ42RQ#discussion_r313110331>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUN6GYNJE45ACSC3OYGH43QEHAYLANCNFSM4IK4KPYQ>
.
|
Oooh wait sorry, I think this was to prevent clipping of component elevation shadows |
Ah nice that sounds legit 👌👌👌
…On Mon, 12 Aug 2019, 21:48 Nick Rout, ***@***.***> wrote:
🤔 I've only seen it on scrolling containers, I didn't think it'd work on
the constraint layout.
… <#m_-7178712554514617139_>
On Mon, 12 Aug 2019, 21:20 Nick Rout, *@*.*> wrote: @.** commented on
this pull request. ------------------------------ In
resources/src/main/res/layout/activity_theme_playground.xml <#27 (comment)
<#27 (comment)>>: > @@
-0,0 +1,243 @@ + +<ScrollView + xmlns:android="
http://schemas.android.com/apk/res/android" + xmlns:tools="
http://schemas.android.com/tools" + xmlns:app="
http://schemas.android.com/apk/res-auto" +
android:layout_width="match_parent" + android:layout_height="match_parent"
+ tools:context=".ThemePlaygroundActivity"> + +
<androidx.constraintlayout.widget.ConstraintLayout +
android:layout_width="match_parent" + android:layout_height="wrap_content"
+ android:paddingBottom="16dp" + android:clipToPadding="false"> Oh lol, so
the components on the playground screen can scroll past the padding without
being clipped — You are receiving this because you were mentioned. Reply to
this email directly, view it on GitHub <#27
<#27>?email_source=notifications&email_token=AAUN6G276HAYTAE4F72SDMLQEHAYLA5CNFSM4IK4KPY2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJ42RQ#discussion_r313110331>,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAUN6GYNJE45ACSC3OYGH43QEHAYLANCNFSM4IK4KPYQ
.
Oooh wait sorry, I think this was to prevent clipping of component
elevation shadows
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#27?email_source=notifications&email_token=AAUN6G6FZKW7J5KMQ2GSP43QEHECDA5CNFSM4IK4KPY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4DZAUQ#issuecomment-520589394>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUN6G4EJFW2BRU6CVO4XU3QEHECDANCNFSM4IK4KPYQ>
.
|
This adds a new module -
:resources
- which is now an:app
dependency, allowing it to also be used by dynamic feature modules.The
:resources
module contains:ThemePlaygroundActivity
(which can now be launched fromMainActivity
)Note: Both the color palette and custom font were chosen by inspecting values on https://letterboxd.com. These can be fairly easily changed if necessary.
resolves #26