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 resources module with themes/styles #27

Merged
merged 9 commits into from
Aug 12, 2019
Merged

Conversation

ricknout
Copy link
Collaborator

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:

  • App themes that inherit from MDC-Android themes
  • Values for color, type and shape theming
  • A downloadable font: https://fonts.google.com/specimen/Poppins
  • A ThemePlaygroundActivity (which can now be launched from MainActivity)
  • Dark theme compatibility via DayNight and the -night resource qualifier

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.

muvi

resolves #26

@ricknout ricknout added the enhancement New feature or request label Aug 11, 2019
@ricknout ricknout requested review from ataulm and hitherejoe August 11, 2019 17:45
@ricknout ricknout self-assigned this Aug 11, 2019
Copy link
Owner

@ataulm ataulm left a 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?

thank you!


import androidx.appcompat.app.AppCompatActivity

class ThemePlaygroundActivity : AppCompatActivity(R.layout.activity_theme_playground)
Copy link
Owner

Choose a reason for hiding this comment

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

very pleased

implementation libraries.androidxAppCompat
implementation libraries.androidxConstraintLayout
implementation libraries.kotlinStdLib
implementation libraries.materialComponents
Copy link
Owner

Choose a reason for hiding this comment

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

this guy alphabetizes


class MainActivity : AppCompatActivity() {
class MainActivity : AppCompatActivity(R.layout.activity_main) {
Copy link
Owner

Choose a reason for hiding this comment

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

cool cool cool

app/build.gradle Outdated
@@ -21,6 +21,8 @@ android {
dependencies {
implementation project(':core')
implementation project(':feed')
implementation project(':navigation')
implementation project(':resources')
Copy link
Owner

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

dependencies.gradle Outdated Show resolved Hide resolved

<application>

<activity android:name=".ThemePlaygroundActivity" />
Copy link
Owner

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?

Copy link
Collaborator

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">
Copy link
Owner

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">
Copy link
Owner

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?

Copy link
Collaborator Author

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

Copy link
Owner

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 😅

Copy link
Collaborator Author

@ricknout ricknout Aug 12, 2019

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 -->
Copy link
Owner

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">
Copy link
Owner

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).

Copy link
Collaborator Author

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?

Copy link
Owner

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

@ataulm
Copy link
Owner

ataulm commented Aug 11, 2019

:resources - which is now an :app dependency, allowing it to also be used by dynamic feature modules.

🤔 resources is an implementation dependency of app so my expectation was that resources would not be available to anyone except app.

In film_detail (DFM), it can't resolve the theme in the IDE:

image

but it works at runtime, and compiles fine.

If we change it to api, then the IDE stops complaining:

image

I think it ought to be api. The good thing about keeping resources as a separate module (instead of consuming it into app is that a design_library module can depend on it too, without depending on app.

@ataulm
Copy link
Owner

ataulm commented Aug 11, 2019

(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 😂 )

@hitherejoe
Copy link
Collaborator

Wow, thanks so much for putting this together @ricknout ! Looks awesome, we'll be glad to no longer see our plain UI when working on this 😆 Looks good from my end, so feel free to merge this @ataulm !

@ataulm ataulm merged commit 0fb6344 into master Aug 12, 2019
@ataulm ataulm deleted the feature/themes-styles branch August 12, 2019 20:21
@ataulm
Copy link
Owner

ataulm commented Aug 12, 2019 via email

@ricknout
Copy link
Collaborator Author

🤔 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 @@ + +<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

@ataulm
Copy link
Owner

ataulm commented Aug 12, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Themes & styles
3 participants