-
Notifications
You must be signed in to change notification settings - Fork 157
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
Adding SwiftUI implementation of Menu List #334
Conversation
looking at the screenshot this doesn't seem right.
|
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 file should not be part of the change:
tools/sgen/Pods/Target Support Files/Pods-sgen/Pods-sgen-frameworks.sh
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
tools/sgen/input/FluentUIStyle.yml
Outdated
padding: $Spacing.medium | ||
textFont: $Typography.body | ||
subtitleFont: $Typography.subheadline | ||
iconColor: [ iconOnly: $Colors.Foreground.neutral3, withLabel: $Colors.Foreground.neutral4, disclosure: $Colors.Foreground.neutral4 ] |
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 think the variations in the figma should be appearance proxies that extend MSFListTokens.
Once we get the state colors like pressed, hover, etc... this will be a matrix of styles vs states for this color.
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.
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
… into sople/listVnext
} | ||
|
||
private func updateLayout(subtitle: String?) -> MSFListCellVnextLayoutType { | ||
if subtitle != "" { |
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.
Are we covering the three line scenario?
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.
As of now, no. We do not have the tokens defined yet, but @Victa should be following up with them later when we need them
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.
Sounds good! I think it's worth creating a GitHub issue to track that work item.
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.
@objc(MSFListVnextCellData) | ||
public class MSFListVnextCellData: NSObject, ObservableObject, Identifiable { | ||
public var id = UUID() | ||
@objc @Published public var leadingIcon: UIImage? |
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.
We'll soon need to add a view as an option to the image (Avatar cell scenario).
Can you create a GitHub issue to track that work?
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.
public var id = UUID() | ||
@objc @Published public var cells: [MSFListVnextCellData] = [] | ||
@objc @Published public var title: String? | ||
@objc @Published public var layoutType: MSFListCellVnextLayoutType = .oneLine |
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.
Wouldn't it be nicer to allow the calling code to specify the layout type per cell (in the MSFListVnextCellData)?
We can discuss this after your first merge (it's worth a GitHub issue to track as well).
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 wanted to consider both cell design + consistency... should we allow sections to contain cells of different heights or lead users into sticking to uniformity? I agree we should continue this in a discussion, I will create an issue
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.
case twoLines | ||
case threeLines | ||
|
||
var height: CGFloat { |
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.
Another thing that can be changed later (GitHub issue):
These values should be tokenized in the YAML file.
Something like:
AP_MSFListTokens:
cellHeight: [ oneLine: 48, twoLines: 64, threeLines: 84 ]
...
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.
Since you added the tokens for these, the snippet below should be removed and you should use the tokenized value instead:
var height: CGFloat {
switch self {
case .oneLine:
return 48
case .twoLines:
return 64
case .threeLines:
return 84
}
}
} | ||
} | ||
|
||
extension MSFListView { |
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.
What's the benefit of having these types in the MSFListView extension?
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.
It was for organization purposes, but I'm thinking of splitting the file (which will be tracked in a GH issue)
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.
ios/FluentUI.Demo/FluentUI.Demo/Demos/ObjectiveCDemoController.m
Outdated
Show resolved
Hide resolved
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.
Just some last nit comments, but overall looking good! :)
Also, can you remove Pods-sgen-frameworks.sh from the PR?
This is just about the execution permission that won't make any difference if checked in.
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
let demoControllerView: UIView = self.view | ||
demoControllerView.addSubview(listView) | ||
|
||
NSLayoutConstraint.activate([demoControllerView.safeAreaLayoutGuide.topAnchor.constraint(equalTo: listView.topAnchor), |
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.
If you are using a custom constraint it would a good idea to not inherit from DemoController
and just use UIViewController
ios/FluentUI.Demo/FluentUI.Demo/Demos/ListVnextDemoController.swift
Outdated
Show resolved
Hide resolved
Thanks for addressing all the comments! :) |
Platforms Impacted
Description of changes
This PR contains the first draft of List (counterpart of UITableView from UIKit), created in SwiftUI. Currently, it supports leading icons, title, and subtitle properties.
Verification
Pull request checklist
This PR has considered:
Microsoft Reviewers: Open in CodeFlow
Microsoft Reviewers: Open in CodeFlow