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

TextTheme Color not Working in V7 #151

Closed
todd-theTello opened this issue May 11, 2023 · 8 comments · Fixed by #153
Closed

TextTheme Color not Working in V7 #151

todd-theTello opened this issue May 11, 2023 · 8 comments · Fixed by #153
Assignees
Labels
bug Something isn't working regression The reported issue is known to have worked correctly before, but no longer does in current release.
Milestone

Comments

@todd-theTello
Copy link

After upgrading to v7 of flex color scheme, text colors defines for various TextThemes don't get applied and I have

image

In order to get it to work I have to specify the color as seen below. This wasn't the issue in V6 of flex color scheme however.

image
@rydmike rydmike self-assigned this May 12, 2023
@rydmike rydmike added the in triage The issue is being tested and verified by replicating it. label May 12, 2023
@rydmike
Copy link
Owner

rydmike commented May 12, 2023

Hi @theTello,

Thanks for the issue report.

Can you provide the above TextTheme snippet as code in a dart code block so I can copy it and investigate further?

textTheme: TextTheme(
  titleLarge: TextStyle(...),
)

@todd-theTello
Copy link
Author

Hello @rydmike
Sure, Below is the snippet


 textTheme: TextTheme(

  /// Default style for buttons in light theme
  titleLarge: GoogleFonts.dmSans(
    fontSize: 28,
    fontWeight: FontWeight.w700,
    height: 32 / 28,
    color: const Color(0xFF2D3436),
  ),

    /// Default style for buttons in light theme
    titleMedium: GoogleFonts.dmSans(
      fontSize: 24,
      fontWeight: FontWeight.w700,
      height: 28 / 24,
      color: const Color(0xFF2D3436),
    ),

    /// Default style for buttons in light theme
    titleSmall: GoogleFonts.dmSans(
      fontSize: 20,
      fontWeight: FontWeight.w500,
      height: 24 / 20,
      color: const Color(0xFF2D3436),
    ),

    /// Default style for buttons in light theme
    labelLarge: GoogleFonts.dmSans(
      fontSize: 18,
      fontWeight: FontWeight.w500,
      height: 20 / 18,
      color: Colors.white,
    ),

    /// Default style for Large body texts
    bodyLarge: GoogleFonts.dmSans(
      fontSize: 16,
      fontWeight: FontWeight.w400,
      height: 20 / 16,
      color: const Color(0xFF2D3436),
    ),

    /// Default style for small body texts
    bodyMedium: GoogleFonts.dmSans(
      fontSize: 14,
      fontWeight: FontWeight.w400,
      height: 18 / 14,
      color: const Color(0xFF636E72),
    ),

    /// Default style for small body texts
    bodySmall: GoogleFonts.dmSans(
      fontSize: 12,
      fontWeight: FontWeight.w400,
      height: 16 / 12,
      color: const Color(0xFF636E72),
    ),
  ),


@rydmike
Copy link
Owner

rydmike commented May 12, 2023

Thanks, I will look into tonight when I'm not at the day job 😄

@rydmike rydmike added bug Something isn't working regression The reported issue is known to have worked correctly before, but no longer does in current release. and removed in triage The issue is being tested and verified by replicating it. labels May 12, 2023
@rydmike
Copy link
Owner

rydmike commented May 12, 2023

Confirmed, it is a regression bug. I thought I had a test case for this usage, but apparently not. Will be adding tests that will hit and test this use case and then applying a fix.

Are you OK with using latest Flutter 3.10 release? Because I probably wont't be back porting this fix to FlexColorScheme v7.0.x. It will only come to latest as FlexColorScheme 7.1.1.

Nice find and thanks for the issue report.

@todd-theTello
Copy link
Author

Okay, I'm comfortable with flutter 3.10.0, Really appreciate all the efforts you're putting in, thank you so much

@rydmike
Copy link
Owner

rydmike commented May 14, 2023

Sorry the fix is taking a bit longer than I thought. It turned out to be a bit bigger than I anticipated.

Would have been easy to just revert. However, I also wanted to keep the feature that automatically give GoogleFont's default textTheme the correct contrast color automatically in FlexColorScheme, while also of course allowing for custom colors, like your case.

I have it all figured out and fixed now, with test for the regression and the new revised GoogleFonts default TextTheme color fix. The test for the regression and additional workaround for the GoogleFonts default color convenience in FCS were also quite extensive. But glad to have those tests added now.

While doing all this, I also started thinking that GoogleFonts package should maybe do a better job, and not force the color from ThemeData.light().textTheme as its default color in a TextTheme, it should hand that over to ThemeData to apply.

Well I say perhaps, because if/when it is used in ThemeData created, it needs a color. Still going to raise an issue that maybe there should be a new null option suitable for use in ThemeData and its textTheme and primaryTextTheme because the current one is not. Due to the way the its default is based on ThemeData.light().textTheme its styles do not have fontSize now, so that might also support that it should not have color either. Size comes via the localization step, it is not applied yet in ThemeData alone.

Anyway, I want to have the GoogleFonts raised issue referenced in the change-log for this fix as well. So next step is to raise that issue. Then merge this fix, and publish new package version 7.1.1 of FCS.

If GoogleFonts will change their default color is another matter, if they do then GoogleFonts TextTheme will work better also when used with vanilla ThemeData and its textTheme and primaryTextTheme properties. If that happen I can later remove some of my own version of the thing that detect if it if a GoogleFonts default TextTheme and null's its colors, so it get correct color and contrast defaults.

@rydmike
Copy link
Owner

rydmike commented May 15, 2023

I have now also added the described GoogleFonts color issue to the GoogleFonts repo. The one about the strange hard coded color and need to work around it when using it in ThemeData to get correct default colors. I added a ref to it in the change log for this issue as well.

The GoogleFonts issue can be found here material-foundation/flutter-packages#401 if you want to take a look.

I have not idea if it is something Material team is willing to change. It is not relevant for the solution of this issue, only for my workaround to deal with the GoogleFonts color issue. I do however think my workaround can remain as is as well. Plus it is another convenience of FlexColorScheme when combined with GoogleFonts, that vanilla ThemeData users will not have, unless Material team fixes/change GoogleFonts as I suggested in the issue.

I am ready to release this fix. will do this evening, after some sleep, so I can double check everything once more with fresh eyes. But it is ready to go 😄

@rydmike
Copy link
Owner

rydmike commented May 15, 2023

Version 7.1.1 with the fix is up on pub, web apps (Playground) still being built by GitHub actions CI/CD, but should be up in 15 mins or so.

Changelog:
https://pub.dev/packages/flex_color_scheme/changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression The reported issue is known to have worked correctly before, but no longer does in current release.
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants