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

Extend ListItem to support overrideTokens #2047

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

joannaquu
Copy link
Contributor

@joannaquu joannaquu commented Jun 17, 2024

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

Extending ListItem to support .overrideTokens so that consumers can customize tokens.

  • Update ListItem so that every time the body runs, the tokenSet will be recreated with the current tokenOverrides
  • Update ListItemDemoController_SwiftUI to use the .overrideTokens modifier
    • .overrideTokens requires that listItem be mutable so move this to a separate var.
  • Remove the extension in ListItemDemoController
    • This was currently updating the fluent theme for the TableViewCellTokenSet, which reflected in the list item demo controller. Since this is not the "right" way to override list item tokens, let's remove it so consumers don't try this as well.
  • Add leadingContentSize as a var in ListItem so that when leadingContentSize changes, we update our tokens that rely on customViewSize.
    • Previously the .leadingContentSize modifier was creating a brand new token set. This meant that any tokens that were overridden would be lost. Now we can ensure that when leadingContentSize, only the tokens dependent on that update.

Binary change

Total increase: 1,344 bytes
Total decrease: -16,168 bytes

File Before After Delta
Total 31,095,248 bytes 31,080,424 bytes 🎉 -14,824 bytes
Full breakdown
File Before After Delta
FocusRingView.o 838,936 bytes 840,280 bytes ⚠️ 1,344 bytes
__.SYMDEF 4,808,816 bytes 4,806,904 bytes 🎉 -1,912 bytes
ListItemModifiers.o 25,856 bytes 20,544 bytes 🎉 -5,312 bytes
ListItem.o 357,984 bytes 349,040 bytes 🎉 -8,944 bytes

Verification

Visual Verification
iOS visionOS
Simulator Screenshot - iPhone 15 Pro - 2024-06-17 at 11 48 53 Simulator Screenshot - Apple Vision Pro - 2024-06-17 at 13 08 54

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)
Microsoft Reviewers: Open in CodeFlow

@joannaquu joannaquu requested a review from a team as a code owner June 17, 2024 20:20
ios/FluentUI/List/ListItem.swift Outdated Show resolved Hide resolved
ios/FluentUI/List/ListItemModifiers.swift Outdated Show resolved Hide resolved
@joannaquu joannaquu changed the title Tokenize ListItem Extend ListItem to support overrideTokens Jun 25, 2024
@joannaquu joannaquu merged commit a36cd3c into microsoft:main Jun 26, 2024
7 checks passed
@joannaquu joannaquu deleted the joannaqu/tokenize-list-item branch June 26, 2024 22:56
}

public var body: some View {
let tokenSet = ListItemTokenSet(customViewSize: { leadingContentSize })
tokenSet.replaceAllOverrides(with: tokenOverrides)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what's the perf impact observed or expected here? We can't guarantee when body runs so hopefully can guarantee this is dirt cheap?

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this offline quite a bit, and yep, token set creation is dirt cheap. We'll work in the future to make it even cheaper, but performance should be totally acceptable right now.

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.

None yet

4 participants