-
Notifications
You must be signed in to change notification settings - Fork 36
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-5354 Enable shadowless cards #1344
Conversation
@@ -43,7 +43,7 @@ public struct BPKCard<Content: View>: View { | |||
} | |||
} | |||
|
|||
private var elevation: Elevation | |||
private var elevation: Elevation? |
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.
Is there a specific reason to make the elevation nullable?
If the user doesn't provide an elevation, we will fallback to 'default'.
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 thought about it like if you want no elevation, this could be represented as nil, as a lack of being a shadow.
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 feel it might be better to be explicit about it.
considering:
- Not supplying = .default
- Making it nil = no shadow
- Passing in .none = no shadow
If we make it non nullable, then we fallback to default and people supply .none to hide the shadow.
|
||
extension View { | ||
@ViewBuilder | ||
func shadow(_ shadow: BPKShadow?) -> some View { |
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 is internal on purpose
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.
Code looks good!
Please update the README.md for both SwiftUI and UIKit components :D
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.
LGTM
Cards without shadow
Given that the apis of SwiftUI and UIKit differ, the solution is different on the 2 systems, with SwiftUI being the preferred one.
Remember to include the following changes:
README.md
Backpack.h
header fileIf you are curious about how we review, please read through the code review guidelines