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

Implement SidebarListStyle #210

Merged
merged 7 commits into from
Jul 29, 2020
Merged

Implement SidebarListStyle #210

merged 7 commits into from
Jul 29, 2020

Conversation

Outcue
Copy link
Contributor

@Outcue Outcue commented Jul 24, 2020

First pass at the SidebarListStyle.

For this to be truly complete, work needs to be completed on ButtonStyles. I can look at that next. NavigationLinks in a Sidebar are Text items that draw a blue background when clicked (and perhaps when item is active.)

@Outcue
Copy link
Contributor Author

Outcue commented Jul 24, 2020

I couldn't figure out how to add the label in order to pass the check-labels check.

@MaxDesiatov MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 24, 2020
@MaxDesiatov MaxDesiatov requested review from j-f1 and carson-katri July 24, 2020 10:31
MaxDesiatov
MaxDesiatov previously approved these changes Jul 24, 2020
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@MaxDesiatov MaxDesiatov changed the title Implement SidebarListStyle Implement SidebarListStyle Jul 24, 2020
@j-f1
Copy link
Member

j-f1 commented Jul 24, 2020

Can you also remove the dividers?

@Outcue
Copy link
Contributor Author

Outcue commented Jul 24, 2020

Can you also remove the dividers?

Sure. Let me take a look.

@Outcue
Copy link
Contributor Author

Outcue commented Jul 24, 2020

I added a check for usage of the SidebarListStyle. This isn't my favorite way of checking capabilities, but I was unsure if I should add some sort of private protocol for "showsDividers" or not.

@j-f1
Copy link
Member

j-f1 commented Jul 24, 2020

I think that's a good way for now and we can re-evaluate later on if needed.

@j-f1 j-f1 linked an issue Jul 24, 2020 that may be closed by this pull request
@MaxDesiatov MaxDesiatov requested a review from j-f1 July 24, 2020 21:38
MaxDesiatov
MaxDesiatov previously approved these changes Jul 24, 2020
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

j-f1
j-f1 previously approved these changes Jul 24, 2020
@@ -120,7 +120,7 @@ public struct _ListRow {
public static func listRow<V: View>(_ view: V, _ style: ListStyle, isLast: Bool) -> some View {
(style as? ListStyleDeferredToRenderer)?.listRow(view) ??
AnyView(view.padding([.trailing, .top, .bottom]))
if !isLast {
if !isLast && style as? SidebarListStyle == nil {
Copy link
Member

Choose a reason for hiding this comment

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

You should probably just add a var addDividers: Bool { get } to the ListStyleDeferredToRenderer protocol, then provide a default value of true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added "hasDividers" to the ListStyle. It wasn't clear how to add it to ListStyleDeferredToRenderer and have the code be clean.

Here is where the property is checked:

@ViewBuilder
  public static func listRow<V: View>(_ view: V, _ style: ListStyle, isLast: Bool) -> some View {
    (style as? ListStyleDeferredToRenderer)?.listRow(view) ??
      AnyView(view.padding([.trailing, .top, .bottom]))
    if !isLast && style.hasDividers {
      Divider()
    }
  }
}

@Outcue Outcue dismissed stale reviews from j-f1 and MaxDesiatov via 05378b9 July 27, 2020 02:42
@MaxDesiatov MaxDesiatov requested review from carson-katri and j-f1 July 27, 2020 08:36
@MaxDesiatov MaxDesiatov merged commit 8041732 into TokamakUI:main Jul 29, 2020
@MaxDesiatov
Copy link
Collaborator

Thank you @Outcue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

Implement SidebarListStyle
6 participants