-
Notifications
You must be signed in to change notification settings - Fork 172
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
Move FluentTheme
code to new FluentUI_common
module
#2139
base: main
Are you sure you want to change the base?
Conversation
Sources/FluentUI_common/Core/Extensions/NSColor+Extensions.swift
Outdated
Show resolved
Hide resolved
Sources/FluentUI_common/Core/Extensions/UIColor+Extensions.swift
Outdated
Show resolved
Hide resolved
Sources/FluentUI_common/Core/Extensions/NSColor+Extensions.swift
Outdated
Show resolved
Hide resolved
self.init(dynamicColor) | ||
} else { | ||
#if os(macOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a good excuse to rename Color+Extensions.swift
to something more descriptive and then introduce something like DynamicColor+UIKit
and DynamicColor+AppKit
(or similar names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the appeal, but two reasons I'm inclined to keep the current names for now:
Color+Extensions.swift
isn't purely based aroundDynamicColor
; it also addsColor.init(hexValue:)
. In fact, the public API is half and half, and doesn't even mentionDynamicColor
at all.- Our convention in Fluent is to name extension files after the class they're extending. Since the files in question extend
Color
,NSColor
, andUIColor
, they should all be named[Color|NSColor|UIColor]+Something.swift
. We could change the+Something
part, but we run into the same problem that there are multiple kinds of additions to each class, not justDynamicColor
.
@@ -3,8 +3,8 @@ | |||
// Licensed under the MIT License. | |||
// | |||
|
|||
#if canImport(UIKit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? Or can we just make this file not compile for macOS?
@@ -6,30 +6,31 @@ | |||
import SwiftUI | |||
|
|||
/// A container that stores a dynamic set of `Color` values. | |||
struct DynamicColor: Hashable { | |||
@objc(MSFDynamicColor) | |||
public final class DynamicColor: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That a bunch of stuff (well actually not that much, but not zero) now has to be exposed publicly when it wasn't before makes me second guess myself. There's no way to share code selectively across platforms with use of path includes/excludes in Package.swift/MicrosoftFluentUI.podspec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, is it actually gross to do this without introducing a new module?
Platforms Impacted
Description of changes
This PR lights up
FluentTheme
and various other low-level Fluent utilities on macOS by moving them to a new cross-platform module,FluentUI_common
. To walk through the changes, I've split the diff into four commits.320e374: Create FluentUI_common target
Package.swift
to specify a new build target,FluentUI_common
.FluentUI_macos
andFluentUI_ios
to depend on this new target.e6b2370: Move common files to FluentUI_common directory
FluentTheme
- and token-related files to the newFluentUI_common
directory, with almost no changes.Color+Extensions.swift
, which hooks into the new macOS-specificDynamicColor
integration found inNSColor+Extensions.swift
.FluentUI.swift
, which now also exportsFluentUI_common
in addition to the platform-specific module. This means users can continue to simplyimport FluentUI
and get the code they expect.069ec24: Add import FluentUI_common to iOS files
import FluentUI_common
to every remaining file inFluentUI_ios
, now that so much of the support structure of Fluent iOS has been moved to that library.1e07c89: Add import FluentUI_common to tests
Binary change
TBA, will be testing the built app to see impact.
Verification
Ran all automation on both iOS and macOS, and got the same level of success as before my change (there are some failing Notification tests that we should fix later).
Also added a direct
FluentTheme
reference in the new FluentUI demo app, demonstrating the ability to access Fluent colors from macOS.Pull request checklist
This PR has considered:
Microsoft Reviewers: Open in CodeFlow