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

Implement AppThemeColor, AppThemeObject, and AppThemeResource #1264

Merged
merged 41 commits into from
Jul 27, 2023

Conversation

jfversluis
Copy link
Member

@jfversluis jfversluis commented Jun 23, 2023

Still todo:

This adds the AppThemeColor and AppThemeResource objects that make it easier to work with colors and other resources in light/dark theme scenarios. This enables you to define colors (and resources) in resource dictionaries, making your XAML less verbose and more reusable.

Original PR on .NET MAUI by Dan Siegel: dotnet/maui#8839

@jfversluis jfversluis added the needs discussion Discuss it on the next Monthly standup label Jun 23, 2023
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Hey Gerald! I've only done a partial review, but at first glance it looks like we can simplify AppThemeColor.cs a bit. What do you think?

src/CommunityToolkit.Maui/AppThemeColor.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeColor.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeColor.shared.cs Outdated Show resolved Hide resolved
@jfversluis
Copy link
Member Author

@brminnick yep thanks! That was my outstanding comment on refactoring it to use the same code. Applied that now. There is one base AppThemeObject<T> and 2 convenience objects basically that inherit from it, but don't add anything except for making it easier to work with in both code and XAML: AppThemeResource which just uses an object and AppThemeColor which does the same for colors. What do you think?

I've left AppThemeObject<T> public on purpose, maybe people want to extend it for their own things?

Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

This looks good in general, I've feel comments around the AppThemeObject class

src/CommunityToolkit.Maui/AppThemeObjectOfT.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeObjectOfT.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeObjectOfT.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeResource.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeColor.shared.cs Outdated Show resolved Hide resolved
src/CommunityToolkit.Maui/AppThemeResource.shared.cs Outdated Show resolved Hide resolved
@jfversluis jfversluis added the pending documentation This feature requires documentation label Jun 25, 2023
@jfversluis
Copy link
Member Author

OK I think I have all the feedback processed. Then the only question remains if we want to rename AppThemeExtension to anything.

If you start typing <Label TextColor="{AppTheme ... it will suggest the AppTheme type in .NET MAUI. Is that a problem? I think ideally we'd pick a different name to avoid collisions. On the other hand, people will most likely add the xmlns first and then type toolkit:AppTheme... and it will suggest the right one?

@jfversluis
Copy link
Member Author

Similarly, Themed Resource would reference a resource.

A color is a resource though, even in this code, it literally just inherits from the resource. But this is more about it being clear to the consumer and not the technicalities I guess 😄

@jfversluis jfversluis removed the needs discussion Discuss it on the next Monthly standup label Jul 11, 2023
@jfversluis
Copy link
Member Author

OK went with ThemeResourceExtension then you would write TextColor="{ThemeResource MyColor}" which is in line with StaticResource and DynamicResource was the idea.

@jfversluis jfversluis marked this pull request as ready for review July 11, 2023 13:00
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Hey Gang! I think we should the classes to Theme... now that we renamed ThemeResouceExtension:

  • public class AppThemeResource -> public class ThemeResource
  • public class AppThemeColor -> public class ThemeColor

Renaming will give us better consistency in XAML:

  • Updated: <mct:ThemeResource ... />
    • Current <mct:AppThemeResource ... />
  • Updated: <mct:ThemeColor ... />
    • Current: <mct:AppThemeColor ... />
  • Current: {mct:ThemeResource ...}

@jfversluis
Copy link
Member Author

@brminnick So yeah, funny story, now we're back to square 1.

The extension is called: ThemeResource and now we have an object ThemeResource... 😅

@brminnick
Copy link
Collaborator

Oh no!! 🙈🙃😭

Naming is so hard...

image

@IndianaGary
Copy link

IndianaGary commented Jul 23, 2023 via email

@brminnick brminnick changed the title Implement AppThemeColor and AppThemeResource Implement AppThemeColor, AppThemeObject, and AppThemeResource Jul 23, 2023
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Thanks Gerald!! I think we finally got the naming:

public sealed class AppThemeColor : AppThemeObject<Color>;

public sealed class AppThemeObject : AppThemeObject<object>;

public abstract class AppThemeObject<T>;

public sealed class AppThemeResourceExtension : IMarkupExtension<BindingBase>;

I approve, and I'll defer to you to merge it to give you time to review the updates.

👇 Here's how these names look in the Sample app

Screenshot 2023-07-23 at 1 16 38 PM

@brminnick brminnick enabled auto-merge (squash) July 23, 2023 20:39
@brminnick brminnick disabled auto-merge July 23, 2023 20:39
@brminnick brminnick merged commit c04539c into main Jul 27, 2023
8 checks passed
@brminnick brminnick deleted the appthemecolor branch July 27, 2023 17:32
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

Successfully merging this pull request may close these issues.

None yet

5 participants