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

Decouple Label and TextColorStyle #2

Merged

Conversation

amgleitman
Copy link
Owner

Platforms Impacted

  • iOS
  • macOS

Description of changes

NOTE: This is currently targeted for new-2ltv, but it will be merged directly into the branch that's part of microsoft#1731 after #1 makes it in.

The discussion of microsoft#1731 uncovered a bug where label tokens don't always play nicely when placed inside parent components. The ultimate problem here is that right now, a Label must be defined in terms of a TextColorStyle. This not only limits what we and consumers of FUA can do with Labels, but it's also an unnecessary abstraction since we usually specify labels in terms of individual typography styles and colors.

Our solution is to make it such that Label is more closely connected to UIColor. This gives us more flexibility in terms of what colors a label can be. It also removes the need for TextColorStyle.secondaryOnColor, which we needed to add as a special case for TwoLineTitleView.

Note that we specify the color as a (FluentTheme) -> UIColor instead of just a UIColor because brand colors (as opposed to neutral colors like "Grey 42" and shared colors like "Blue Primary") require a theme in order to be translatable to a UIColor. See the usage in LabelDemoController.swift for an example.

Verification

(how the change was tested, including both manual and automated tests)

Visual Verification
Light mode Dark mode Token override*
Simulator Screen Shot - iPhone 14 Pro - 2023-05-11 at 13 30 39 Simulator Screen Shot - iPhone 14 Pro - 2023-05-11 at 13 30 43 Simulator Screen Shot - iPhone 14 Pro - 2023-05-11 at 13 30 25

*This was created by manually changing TwoLineTitleViewTokenSet to show UIColor.systemRed for the title and UIColor.systemOrange for the subtitle. This obviously isn't part of the change, but it's proof of concept that it works.

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)

Copy link

@laminesm laminesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these changes have any impact on Label token overrides?

@amgleitman
Copy link
Owner Author

Do these changes have any impact on Label token overrides?

Nope, Label token overrides still work just fine!

@amgleitman amgleitman requested a review from mischreiber May 11, 2023 22:56
@amgleitman amgleitman changed the base branch from new-2ltv to navbar-merge-main-2023-05-04-minus-1699 May 12, 2023 02:42
@amgleitman amgleitman merged commit 155c4ad into navbar-merge-main-2023-05-04-minus-1699 May 12, 2023
@amgleitman amgleitman deleted the decouple-textcolorstyle branch May 12, 2023 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants