Multi-tenant using config file #637
Conversation
|
Thanks for the pull request, @RawanMatar89! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Hi @RawanMatar89! Just flagging a couple failing checks here. |
OmarIthawi
left a comment
There was a problem hiding this comment.
Thanks @RawanMatar89, I have few notes to reduce the git diff and to ensure we have an easy Developer Experience for someone who's developing and testing on a single Tenant.
We also need to document the main changes done in this pull request in a document in this repository for iOS-specific changes as complimentary document to the OEP.
@IvanStepanok @volodymyr-chekyrta please give this a quick review so we continue on polishing it in parallel with the OEP.
| .tint(themeManager.theme.colors.infoColor) | ||
| .foregroundStyle(themeManager.theme.colors.textSecondaryLight) |
There was a problem hiding this comment.
I think we should avoid adding uneeded diff, so the Theme class should be updated at runtime in-place:
| .tint(themeManager.theme.colors.infoColor) | |
| .foregroundStyle(themeManager.theme.colors.textSecondaryLight) | |
| .tint(Theme.colors.infoColor) | |
| .foregroundStyle(Theme.colors.textSecondaryLight) |
| let cssInjector: CSSInjector | ||
| let proxy: GeometryProxy | ||
| @Environment(\.colorScheme) var colorScheme | ||
| @EnvironmentObject var themeManager: ThemeManager |
There was a problem hiding this comment.
| @EnvironmentObject var themeManager: ThemeManager |
| self.theme = selectedTheme | ||
| self.theme.name = localizedName |
There was a problem hiding this comment.
I think theme manager should not be updated, but the theme itself should be.
This will reduce the diff for practical reasons.
- self.theme = selectedTheme
- self.theme.name = localizedName
+ Theme.name = localizedName| public final class ThemeManager: ObservableObject { | ||
| public static let shared = ThemeManager() | ||
|
|
||
| @Published public private(set) var theme: ThemeDefinition |
There was a problem hiding this comment.
I recommend merging ThemeDefinition and Theme in a single class.
…into MultiTenant # Conflicts: # Core/Core/Extensions/Notification.swift # Course/Course/Presentation/Container/CourseContainerView.swift # Course/Course/Presentation/Outline/ContinueWithView.swift
|
Hi @RawanMatar89 @OmarIthawi! Is this pull request still in progress? |
Friendly ping on this @RawanMatar89 @OmarIthawi |
hi @mphilbrick211, we’ve slowed down a bit on this PR, but we’re aiming to finish it soon, with the minimum accepted updates. |
Describtion:
This PR introduces multi-tenant support to the app, allowing users to select their institution (tenant) at run-time with distinct configurations, branding.
TODO:
Screenshots:
each tenant could has it own login screen e.g.:
The user can switch between tenant from setting screen:
|Settings screen - switch tenant section|Select a tenant screen|
custom brand-colors for each tenant: