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

Remove Icon.Content and replace it with ViewBuilder parameters #597

Conversation

PavelHolec
Copy link
Collaborator

@PavelHolec PavelHolec commented Jun 15, 2023

This is another task to make call site being able to inject any @ViewBuilder and make our wrapper components more simple and flexible. Now it is updated only for Icons, but follow-up tasks will clean up "disclosure", "buttons" and similar component properties.

The @ViewBuilder Icon closure differs based on components - sometimes, the component does not make sense without an icon, in which case, the closure is not optional.

For some components in future that would contain a large amount of ViewBuilders, we may even extract optional disclosure and other subviews as modifiers, similar to toolbar() modifier.

Inspiration from SwiftUI components

In general, most, if not all, SwiftUI components and modifiers follow these principles:

  • the first argument has no name, if it is the primary property of that component (Label("label", icon:) vs. frame(width:, height:))
  • parameter list ends with:
    • action handlers, sorted from primary action (tap, select) to secondary action (onCancel)
    • @ViewBuilders, sorted from primary content to secondary content (icon)

Label

A title is the primary information (goes first), an icon is secondary. The component is correctly able to detect EmptyView to avoid empty space in the HStack (we are doing the same in components that use Icon, although without access to internals of viewbuilder layout, we cannot correctly detect if/else/foreach empty cases).

// Full customization 
init(@ViewBuilder title: () -> Title, @ViewBuilder icon: () -> Icon)
// Basic init
init(_ title: S, systemImage name: String) where S : StringProtocol

DisclosureGroup

The primary "content" goes as a first @ViewBuilder.

init(isExpanded: Binding<Bool>, @ViewBuilder content: @escaping () -> Content, @ViewBuilder label: () -> Label)

PayWithApplePayButton

Actions go first, then @ViewBuilders

init(_ label: PayWithApplePayButtonLabel = .plain, action: @escaping () -> Void, @ViewBuilder fallback: () -> Fallback)

@PavelHolec PavelHolec added the enhancement New feature or request label Jun 15, 2023
@PavelHolec PavelHolec added this to the 2023Q2 milestone Jun 15, 2023
@PavelHolec PavelHolec self-assigned this Jun 15, 2023
@PavelHolec PavelHolec linked an issue Jun 15, 2023 that may be closed by this pull request
@PavelHolec PavelHolec mentioned this pull request Jun 15, 2023
@PavelHolec PavelHolec force-pushed the 525-remove-iconcontent-and-replace-it-with-viewbuilder-parameters branch 2 times, most recently from fd8b2ed to 3191cff Compare June 16, 2023 11:20
@PavelHolec PavelHolec requested a review from sjavora June 16, 2023 11:31
@PavelHolec PavelHolec marked this pull request as ready for review June 16, 2023 11:35
@PavelHolec PavelHolec requested a review from a team as a code owner June 16, 2023 11:35
Sources/Orbit/Components/Alert.swift Show resolved Hide resolved
Sources/Orbit/Components/Badge.swift Show resolved Hide resolved
Sources/Orbit/Components/Badge.swift Outdated Show resolved Hide resolved
Sources/Orbit/Components/Button.swift Outdated Show resolved Hide resolved
Sources/Orbit/Components/Button.swift Show resolved Hide resolved
Sources/Orbit/Support/Components/IconButton.swift Outdated Show resolved Hide resolved
Snapshots/iPhone/CardTests/testCards.1.png Outdated Show resolved Hide resolved
@PavelHolec PavelHolec force-pushed the 525-remove-iconcontent-and-replace-it-with-viewbuilder-parameters branch from 17b3a5e to a19eae5 Compare June 20, 2023 12:02
@PavelHolec PavelHolec merged commit 16142a1 into main Jun 20, 2023
@PavelHolec PavelHolec deleted the 525-remove-iconcontent-and-replace-it-with-viewbuilder-parameters branch June 20, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Icon.Content and replace it with ViewBuilder parameters
2 participants