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

GetCurrentThemeNameAsync() -> GetSiteThemeNameAsync() #12067

Merged
merged 1 commit into from Aug 3, 2023

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Jul 23, 2022

This change makes ISiteThemeService similar to IAdminThemeService in terms of APIs

@Skrypt
Copy link
Contributor

Skrypt commented Jul 26, 2022

The difference here is that SiteTheme is a tenant-scoped service. Maybe it would be better that the name reflects it.
Just an idea. Or add a comment somewhere about it.

@hishamco
Copy link
Member Author

The difference here is that SiteTheme is a tenant-scoped service.

Also the AdminTheme is tenant-scoped, while we can set admin theme per tenant, Am I right?

The context here is to make both classes has similar APIs while the difference is one sets the site theme and the other sets the admin theme

@Skrypt
Copy link
Contributor

Skrypt commented Jul 27, 2022

You are right, my brain collapsed. 😄

@Skrypt Skrypt requested a review from jtkech July 27, 2022 06:18
@hishamco
Copy link
Member Author

No problem it happen ;)

@hishamco hishamco requested review from Skrypt and removed request for jtkech September 6, 2022 18:16
@Skrypt
Copy link
Contributor

Skrypt commented Sep 7, 2022

I approve once we agree on the new name.

@hishamco
Copy link
Member Author

hishamco commented Sep 7, 2022

@sebastienros are you agree with the name? We should be consistent in terms of naming APIs

@sebastienros sebastienros merged commit 2217b75 into main Aug 3, 2023
@sebastienros sebastienros deleted the theme-service branch August 3, 2023 17:50
@hishamco
Copy link
Member Author

hishamco commented Aug 3, 2023

Finally merged :)

@MikeAlhayek
Copy link
Member

you're welcome lol. It pays to join the meeting

@hishamco
Copy link
Member Author

hishamco commented Aug 4, 2023

Hope to join you in the upcoming meetings.

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.

None yet

4 participants