garretyoder / Colorful

Android runtime theme library
Apache License 2.0
2.13k stars 193 forks source link

Support for custom theme extensions added #29

Closed MFlisar closed 6 years ago

MFlisar commented 6 years ago

Done

How it works

ThemeColor is replaced by ThemeColorInterface in the whole library.

1) Create styles for the custom theme like following:

    <style name="primary_red_50">
        <item name="android:colorPrimary">@color/md_red_50</item>
        <item name="colorPrimary">@color/md_red_50</item>
    </style>

    <style name="primary_dark_red_50">
        <item name="android:colorPrimaryDark">@color/md_red_50</item>
        <item name="colorPrimaryDark">@color/md_red_50</item>
    </style>

    <style name="accent_red_50">
        <item name="android:colorAccent">@color/md_red_50</item>
        <item name="colorAccent">@color/md_red_50</item>
    </style>

2) Create a CustomThemeColor object like following (or use one of the alternative constructors):

    var primaryColor = CustomThemeColor(
            context,
            R.style.primary_red_50,
            R.style.accent_red_50,
            R.color.md_red_50,
            R.color.md_red_50
    )

3) simply use this primaryColor object like you used e.g. ThemeColor.RED before

What do you think about this? Do you consider this feature useful as well?

MFlisar commented 6 years ago

Important question

You use the resource ids to persist user selections in preferences. Is this really safe? I do this in my extensions I made in this pull request as well.

Those ids are not 100% sure to never change after builds as far as I know (so it's not safe to assume, that those persisted ids are valid after a rebuild of the app). We could persist the resource string names instead and use them to read back in the settings. What do you think about this? Sadly this would be a breaking change...

garretyoder commented 6 years ago

I'll take a look at the PR later today, but the resource ids is a good point. In the old legacy version of colorful I had it loading by resource name versus ID. I don't think it'd be too bad a of a breaking change, at worst the persisted settings are lost. I'll look into it today as well.

garretyoder commented 6 years ago

Could you update the readme in your PR to document these new features? I'm most likely going to approve this, it's a good change.

I wonder if we could allow the user to implement their own ThemeColor interface so they could have easy syntax such as CustomThemes.Magenta

MFlisar commented 6 years ago

Extended the readme. Actually, you can use ANY class that implements ThemeColorInterface, it must not be a CustomThemeColor, this class only exists for convenience...

It should be no problem to define a custom enum that implements the ThemeColorInterface that's named CustomThemes and define user enums like Magenta in there

garretyoder commented 6 years ago

I didn't get a chance to look at this tonight, I was busy trying to fix a bug in JRAW, however I will take a look tomorrow and merge. If I have any quibbles it will be in class naming only, this PR looks good.