Theme Services

[AathifMahir]

Pull Request

Theme Services that Merges Most Used Design Tokens to Create Centralized Theme System Where User can just Inject Theme Service and Do the Theme Changes Instead Inject Lots of Different Design Tokens

📖 Description

This pull request makes easy for users to do the theme changes without much of context

📑 Test Plan

✅ Checklist

General

• [ ] I have added tests for my changes.
• [ ] I have tested my changes.
• [ ] I have updated the project documentation to reflect my changes.
• [x] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• [ ] I have added a new component
• [ ] I have added Unit Tests for my new compontent
• [ ] I have modified an existing component
• [ ] I have validate Unit Tests for an existing component

⏭ Next Steps

[AathifMahir]

@vnbaaij This is initial Prototype of Theme Service that i talked about, I would like you opinion on that, Therefore i can move forward on this start readying this for production and also i would like to listen to your opionion on Best Place to Initialize the Theme Service, Are we giving users options to initialize the theme service in there desired layout or maybe creating a ThemeContainer ourself that does the Initialization

[AathifMahir]

@microsoft-github-policy-service agree

[vnbaaij]

Hi @AathifMahir,

I've looked at your PR with my fellow maintainer @dvoituron. It needs quite a bit more work before we can think of merging this. We do see value in an enhancement like this.

Here are some of the things we noticed on the current state of the PR

• We adhere to certain coding standards. Please look at the rest of the source to see if your style is matching ours. Couple of examples: We don't use Is... or 'Has...as a prefix for parameters, variables or methods.if...then...always have{}`, etc.
• If we want do do persistence on a theme, it needs to be done through an interface so we can also enable storing the data in a cookie, file, local storage, etc.
• The goal of the demo is to showcase as many of the possibilities as possible. The option to select a color for the theme is one of them. Your PR abstract too much of that functionality.
• We are already way behind on having tests for our code. We do not take PRs that do not contain test for the functionality it contains.
• Some of the things you moved in to the theme service js file are very specific for the demo environment (like the isMobile check and the switching of the highlighter stylesheet. These should not be part of a generic service.

We hope this gives you some handles to move forward with this. Thanks for the effort so far!

[AathifMahir]

Hi @AathifMahir,

I've looked at your PR with my fellow maintainer @dvoituron. It needs quite a bit more work before we can think of merging this. We do see value in an enhancement like this.

Here are some of the things we noticed on the current state of the PR

• We adhere to certain coding standards. Please look at the rest of the source to see if your style is matching ours. Couple of examples: We don't use Is... or 'Has...as a prefix for parameters, variables or methods.if...then...always have{}`, etc.
• If we want do do persistence on a theme, it needs to be done through an interface so we can also enable storing the data in a cookie, file, local storage, etc.
• The goal of the demo is to showcase as many of the possibilities as possible. The option to select a color for the theme is one of them. Your PR abstract too much of that functionality.
• We are already way behind on having tests for our code. We do not take PRs that do not contain test for the functionality it contains.
• Some of the things you moved in to the theme service js file are very specific for the demo environment (like the isMobile check and the switching of the highlighter stylesheet. These should not be part of a generic service.

We hope this gives you some handles to move forward with this. Thanks for the effort so far!

Thanks for the reply,

I'll start enhancing this based on the Fluent UI standards

[AathifMahir]

Would you mind clarifying the second point, as that describes that Storage Related stuff done by interface? The current prototype includes an interface to do the persistent/storage or else I'm not catching it? @vnbaaij

[vnbaaij]

Yeah, you have an interface indeed. @dvoituron do you want to chime in here?

[dvoituron]

Hi @AathifMahir I think that the persistence service should be outsourced. It is not the purpose of this library to use LocalStorage. The best would be to have a ThemeSerialized property/method (for example) that allows you to retrieve a json with all the theme data, and update this property to reapply the theme.

I quickly reviewed your code and added a few comments. Please don't hesitate to contact me for further details.

[AathifMahir]

Hi @AathifMahir I think that the persistence service should be outsourced. It is not the purpose of this library to use LocalStorage. The best would be to have a ThemeSerialized property/method (for example) that allows you to retrieve a json with all the theme data, and update this property to reapply the theme.

I quickly reviewed your code and added a few comments. Please don't hesitate to contact me for further details.

Sure, I'll go through it. Thanks 👍

[c0g1t8]

Great code review - learned some things.

[AathifMahir]

@vnbaaij @dvoituron any update on this PR, I have already updated most of the implementation to production ready with some minor exception

[dvoituron]

@vnbaaij @dvoituron any update on this PR, I have already updated most of the implementation to production ready with some minor exception

I haven't had time to finalize this review yet. I'll have a look as soon as I can.

[vnbaaij]

Hi @AathifMahir, I propose we close this PR in favor of what we are discussing here: #1049

[AathifMahir]

Hi @AathifMahir, I propose we close this PR in favor of what we are discussing here: #1049

Seems like it's achieves the same as this one, I guess it makes to sense to close. Go ahead with it 👍