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

Migrating from 2.4.2 to 3.x.x #105

Closed
marcardar opened this issue Apr 17, 2020 · 14 comments
Closed

Migrating from 2.4.2 to 3.x.x #105

marcardar opened this issue Apr 17, 2020 · 14 comments

Comments

@marcardar
Copy link

marcardar commented Apr 17, 2020

My existing code has:

    override fun getViewTypeManager(): ViewTypeManager {
        return AboutViewTypeManager()
    }

But now getViewTypeManager() is private. Is there migration advice on how best to deal with this?

It's not clear why #101 needed to change that method (and some others) from protected to private.

@daniel-stoneuk
Copy link
Owner

Hi, I'll look into this - sorry.

@daniel-stoneuk
Copy link
Owner

Yep, you're right. Sorry I didn't pay enough attention to the PRs in the past - will keep more on top in the future. Will push a release with fixes in a mo

@daniel-stoneuk
Copy link
Owner

Fixed in #245eead, and release 3.1.2.

Cheers. Let me know how the rest of the migration goes - and I'd love to see which app you're using this in.

@marcardar
Copy link
Author

marcardar commented Apr 17, 2020

Cheers Daniel for a super-quick fix! I confirm that the fix works.

The only other issue I'm having is regarding theme, though I'm not sure whether this is caused by switching my app theme to MaterialComponents. For example, I notice icons are now black (in about screen, but also nav drawer), and that background color (in night mode) is now black (was very dark grey).

Note - I'm using fragments (i.e. "single activity" approach) (and so I used to override getTheme() which has since gone), and so styling by activity themes mentioned in your docs, doesn't apply here. For migration, I simply removed my getTheme() implementation:

override fun getTheme() =
    when (resources.configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK) {
        Configuration.UI_MODE_NIGHT_YES -> R.style.Theme_Mal_Dark
        else -> // Configuration.UI_MODE_NIGHT_NO
            R.style.Theme_Mal_Light
    }

p.s. I'm using your library in an internal project - I'll let you know if/when made public!

@daniel-stoneuk
Copy link
Owner

No worries! Glad that it's solved.

Switching to MaterialComponents has changed the dark theme background colour to black and this is independent of the library.

I decided to get rid of the themes since they aren't required and have become outdated. The colour of the icons is from Material Components 'colorOnSurface' attribute so if you want these to be grey, you can change this.

Is the light / dark theming still working in your app? Have you considered using MaterialComponents DayNight?

@marcardar
Copy link
Author

Yes, I'm using the DayNight theme, but according to this, the dark background color shouldn't be black: https://material.io/develop/android/theming/dark/

My other fragments are also showing black background, so yes I don't think this is anything to do with your library.

The colour of the icons is from Material Components 'colorOnSurface' attribute so if you want these to be grey, you can change this.

So I guess applying a ContextThemeWrapper in the onCreateView of the about fragment?

@daniel-stoneuk
Copy link
Owner

Ah, I couldn't find anything on the Android side of the docs regarding the background colour but in the Material Guidelines there is information on dark themes: https://material.io/design/color/dark-theme.html

The new dark theme surface colour is #121212 which is (I think) the black that we're seeing. Another interesting note - Toolbars don't have any elevation or colour in dark theme.

So I guess applying a ContextThemeWrapper in the onCreateView of the about fragment?

Yep, that sounds like the way to go. Hope it works & let me know how it goes.

@marcardar
Copy link
Author

Yes, you're right #121212 is what I'm seeing, though I've checked out a few of Google's apps (Photos, Gmail, Play Store) and they all use #202124 (which IMO looks nicer). Still, good to know the expected colour is appearing. I'll let you know if I find anything else interesting.

BTW: I use my Pixolor app to inspect the colors: https://play.google.com/store/apps/details?id=com.embermitre.pixolor.app

@marcardar
Copy link
Author

marcardar commented Apr 19, 2020

Hi Daniel - I noticed the MaterialCardView is set up with a 0dp elevation. Is there a reason for this, as opposed to using the default (which I think is 1dp).

I noticed, in dark mode, the colour of the card should change according to the elevation, so I wonder if the default elevation was used then the card would/should be a lighter shade of grey compared to the background.

Here is some interesting reading: https://stackoverflow.com/questions/60800857/how-come-the-color-i-set-for-cardbackgroundcolor-isnt-exactly-the-one-that-gets

@daniel-stoneuk
Copy link
Owner

I'd changed the design of the cards to be an outlined card (see: https://material.io/develop/android/components/cards/) since it looks a lot cleaner than the regular cards do in my opinion - the old versions of the library looked quite outdated so I thought I would change up the design whilst changing the version number.

I understand that this might not be for everyone - what do you recommend we do next:

  • Add section to README to show how to override card layout
    • This is done by adding your own layout named mal_material_about_list_card.xml, and copying in the contents from the library & changing to your preference
    • This is quite hacky, and if lots of people want to do this it surely won't be the best option
  • Add a parameter to the MaterialAboutCard to switch between outlined card and non-outlined card.

Unless you suggest both an outline and an elevation? Thanks for the feedback, much appreciated 😄

@daniel-stoneuk daniel-stoneuk changed the title Migrating from 2.4.2 to 3.1.1: private getViewTypeManager() Migrating from 2.4.2 to 3.x.x Apr 19, 2020
@marcardar
Copy link
Author

Thanks again Daniel.

I tried using my own layout and I agree an outline card looks better with no elevation. However, using default elevation and default stroke (no outline), looks even cleaner IMO. Regardless, I'm happy with either of the two solutions you propose, though arguably the second is the best.

@marcardar
Copy link
Author

Apparently this is a good way to set the theme in the fragment.

override fun onGetLayoutInflater(savedInstanceState: Bundle?): LayoutInflater {
    context?.setTheme(R.style.Theme_About)
    return super.onGetLayoutInflater(savedInstanceState)
}

@marcardar
Copy link
Author

Hi Daniel - I noticed the styles used in a couple of the layouts are using AppCompat styles. Is that intentional?

If not, how about:

mal_material_about_list_card
@style/TextAppearance.MaterialComponents.Headline6

mal_material_about_title_item
@style/TextAppearance.MaterialComponents.Headline5
@style/TextAppearance.MaterialComponents.Body2

@daniel-stoneuk
Copy link
Owner

Not intentional - thanks for spotting this. I'll break this into a new issue and fix soon. Cheers.

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

No branches or pull requests

2 participants