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

Replace BaseTheme with AppStyles #167

Merged
merged 8 commits into from
Jan 18, 2023
Merged

Replace BaseTheme with AppStyles #167

merged 8 commits into from
Jan 18, 2023

Conversation

HobDev
Copy link
Contributor

@HobDev HobDev commented Dec 27, 2022

Description of Change

As AppThemeBinding is available now I removed the old way of supporting Themes. Instead I use AddAppThemeBinding in the Styles. The changes are in the Sample app only.

Linked Issues

  • Fixes #

PR Checklist

Additional information

@brminnick
Copy link
Collaborator

brminnick commented Jan 5, 2023

Thanks @HobDev! I really like this approach!

Could you also submit a PR to our docs, adding this? I think a lot of developers would find this really helpful too!

It doesn't look like we have any documentation on theming, so I can see two ways you could approach this problem:

  • Create a new doc page, something like /docs/maui/markup/theming.md
    • I think this would be best to help devs specifically looking for Light Theme / Dark Theme help
  • Add to the existing styles.md doc docs/maui/markup/extensions/style.md
    • Add a ## Theming section here
    • Putting it in the existing Style API docs may bury the info and be tough for devs to find, though

Once you have a PR open for the docs, I'll merge this PR 🙌

(I've added the pending documentation This feature requires documentation and do not merge Do not merge this PR labels to remind me not to accidentally merge this until the docs are done)

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 again @HobDev! Just a few recommendations to keep the color constants (I would put them in AppStyles.cs)

@brminnick brminnick added do not merge Do not merge this PR pending documentation This feature requires documentation labels Jan 5, 2023
@bijington
Copy link
Contributor

I am happy to provide the docs for how to style an application given I should have done so with the previous PR.

@HobDev
Copy link
Contributor Author

HobDev commented Jan 8, 2023

I am happy to provide the docs for how to style an application given I should have done so with the previous PR.

@bijington Thank you for the help. So I need not work on the docs as you are going to write it.

@HobDev
Copy link
Contributor Author

HobDev commented Jan 8, 2023

@brminnick Thank you for the review. I made all colors static readonly in the latest commit. The Colors which are used in Dark Mode have Dark at the end of their name. I am not sure whether this naming is correct. Please let me know if further improvements are required.

@bijington
Copy link
Contributor

Thanks @HobDev! I really like this approach!

Could you also submit a PR to our docs, adding this? I think a lot of developers would find this really helpful too!

It doesn't look like we have any documentation on theming, so I can see two ways you could approach this problem:

  • Create a new doc page, something like /docs/maui/markup/theming.md

    • I think this would be best to help devs specifically looking for Light Theme / Dark Theme help
  • Add to the existing styles.md doc docs/maui/markup/extensions/style.md

    • Add a ## Theming section here
    • Putting it in the existing Style API docs may bury the info and be tough for devs to find, though

Once you have a PR open for the docs, I'll merge this PR 🙌

(I've added the pending documentation This feature requires documentation and do not merge Do not merge this PR labels to remind me not to accidentally merge this until the docs are done)

I have added a PR at MicrosoftDocs/CommunityToolkit#189 strangely I can't seem to assign you as a reviewer @brminnick

@brminnick brminnick added documentation approved and removed do not merge Do not merge this PR pending documentation This feature requires documentation labels Jan 18, 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 @HobDev!!

And thank you @bijington for folding this into the docs! It'll be super helpful for devs in the future 💯

@brminnick brminnick enabled auto-merge (squash) January 18, 2023 18:39
@brminnick brminnick merged commit 0349830 into CommunityToolkit:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants