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

KOA-5072 Add BPKBadge to SwiftUI #1355

Merged
merged 19 commits into from
Jun 30, 2022
Merged

Conversation

gert-janvercauteren
Copy link
Contributor

@gert-janvercauteren gert-janvercauteren commented Jun 28, 2022

We have created the SwiftUI version of the BPKBadge. This version also includes the newer 'strong' and 'normal' styles.

Light mode Dark mode
simulator_screenshot_9B57650E-E065-4EBF-A277-AF59F0973485 simulator_screenshot_EE2896F1-AF2C-4BB9-97B9-EF84F399A6AB

Remember to include the following changes:

If you are curious about how we review, please read through the code review guidelines

@gert-janvercauteren gert-janvercauteren self-assigned this Jun 28, 2022
@gert-janvercauteren gert-janvercauteren added swiftui minor Non breaking change labels Jun 28, 2022
@gert-janvercauteren gert-janvercauteren marked this pull request as ready for review June 28, 2022 08:05
}

private var content: some View {
HStack(spacing: BPKSpacing.sm.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily part of this pr, but may be nice to add an HStack init that accepts bpkspacing to allow for HStack(spacing: .sm)

.foregroundColor(style.foregroundColor)
}
// Align text and icon frame.
.frame(minHeight: BPKSpacing.base.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what would happen (and what we want to happen) if the user has the font size set to Large on their phone. will the text grow and not the icon?

Think the best is to set height instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the min-height will allow the font to grow in the future.

We can also consider setting the height of the whole badge to 24, this follows design but will not allow for spacing or font adjustments.

.padding([.top, .bottom], .sm)
.background(style.backgroundColor)
.clipShape(RoundedRectangle(cornerRadius: .xs))
.overlay(
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make it so that only the outline style adds an overlay?

If it'd be the case that a specific style adds an icon on the right, you wouldn't add it for every style and make it transparent, you would add it just in that case right?

open to debate it tho!

}
}

var borderColor: BPKColor {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this, but see my comment on the bpkbadge file

Comment on lines 87 to 95
guard let color = color else {
return AnyView(self)
}

return AnyView(
ModifiedContent(
content: self,
modifier: Outline(color: color, cornerRadius: cornerRadius)
)
Copy link
Contributor

@frugoman frugoman Jun 29, 2022

Choose a reason for hiding this comment

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

I think this can be a @ViewBuilder, to remove the need for AnyView-ing
like this: https://www.swiftbysundell.com/articles/avoiding-anyview-in-swiftui/

@ViewBuilder
func outline(_ color: BPKColor?, cornerRadius: BPKCornerRadius) -> some View {
  if let color = color {
     self
  } else {
     ModifiedContent(..)
  }
}

@gert-janvercauteren gert-janvercauteren merged commit bcd4aff into main Jun 30, 2022
@gert-janvercauteren gert-janvercauteren deleted the KOA-5072-swiftui-badge branch June 30, 2022 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Non breaking change swiftui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants