-
Notifications
You must be signed in to change notification settings - Fork 5
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
IOS-9604 Added infinite interval to crouton and snackbar for the Swif⦠#324
Conversation
public enum CroutonDismissInterval { | ||
case fiveSeconds | ||
case tenSeconds | ||
case infinite | ||
|
||
var timeInterval: TimeInterval? { | ||
switch self { | ||
case .fiveSeconds: | ||
return 5 | ||
case .tenSeconds: | ||
return 10 | ||
case .infinite: | ||
return nil | ||
} | ||
} | ||
} |
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.
Maybe a more generic approach to ease adding more intervals in the future?
public enum CroutonDismissInterval { | |
case fiveSeconds | |
case tenSeconds | |
case infinite | |
var timeInterval: TimeInterval? { | |
switch self { | |
case .fiveSeconds: | |
return 5 | |
case .tenSeconds: | |
return 10 | |
case .infinite: | |
return nil | |
} | |
} | |
} | |
public enum CroutonDismissInterval { | |
case seconds(TimeInterval) | |
case infinite | |
var timeInterval: TimeInterval? { | |
switch self { | |
case .seconds(let value): | |
return value | |
case .infinite: | |
return nil | |
} | |
} | |
} |
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 can't do this because you let the user to add any interval and its definition does not allow this.
func normalizeDismissInterval(from action: (() -> Void)?, croutonDismissInterval: CroutonDismissInterval?) -> CroutonDismissInterval { | ||
switch croutonDismissInterval { | ||
case .none where action != nil: | ||
return .tenSeconds | ||
case .fiveSeconds where action != nil: | ||
return .tenSeconds | ||
case .tenSeconds where action == nil: | ||
return .fiveSeconds | ||
case .none: | ||
return .fiveSeconds | ||
case .fiveSeconds: | ||
return .fiveSeconds | ||
case .tenSeconds: | ||
return .tenSeconds | ||
case .infinite: | ||
return .infinite | ||
} | ||
} |
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 this behavior specified in any document? It's confusing create a crouton with a dismiss interval of 5 seconds that dissappear after 10 seconds...
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.
our components have a Readme so we can include it to explain this. Good idea!
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 will add this behaviour to the Readme. But JFYI: https://www.figma.com/file/yCHLIfy4WMfRdlADwyL4kZ/πΈ-Snackbar-Specs?type=design&node-id=1038-1306&mode=design&t=H1SNmcgaFocwgXn9-0
@@ -25,38 +25,105 @@ public enum SnackbarButtonStyle { | |||
case short | |||
} | |||
|
|||
public struct Snackbar<Button: View>: View { | |||
public enum SnackbarDismissReason: Int, RawRepresentable { |
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.
could it be moved to MisticaCommon to avoid having the UIKit & SwiftUI representations?
case button | ||
case timeout | ||
|
||
public typealias RawValue = String |
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.
weird...
what about having?
- public enum SnackbarDismissReason: Int
+ public enum SnackbarDismissReason: String
Then, you can assign its String
directly
case dismiss = "DISMISS"
case button = "BUTTON"
case timeout = "TIMEOUT"
you will be able to remove var rawValue
and init?(rawValue: RawValue)
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.
@@ -9,23 +9,44 @@ | |||
import Foundation | |||
import SwiftUI | |||
|
|||
public enum CroutonDismissInterval { |
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.
Repeated in UIKit, could we put it in common?
func normalizeDismissInterval(from action: (() -> Void)?, croutonDismissInterval: CroutonDismissInterval?) -> CroutonDismissInterval { | ||
switch croutonDismissInterval { | ||
case .none where action != nil: | ||
return .tenSeconds | ||
case .fiveSeconds where action != nil: | ||
return .tenSeconds | ||
case .tenSeconds where action == nil: | ||
return .fiveSeconds | ||
case .none: | ||
return .fiveSeconds | ||
case .fiveSeconds: | ||
return .fiveSeconds | ||
case .tenSeconds: | ||
return .tenSeconds | ||
case .infinite: | ||
return .infinite | ||
} | ||
} |
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.
our components have a Readme so we can include it to explain this. Good idea!
func dismiss(with reason: SnackbarDismissReason) { | ||
DispatchQueue.main.async { | ||
withAnimation { isVisible = false } | ||
dismissHandlerBlock?(reason) | ||
} | ||
} |
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 you can do:
func dismiss(with reason: SnackbarDismissReason) { | |
DispatchQueue.main.async { | |
withAnimation { isVisible = false } | |
dismissHandlerBlock?(reason) | |
} | |
} | |
@MainActor | |
func dismiss(with reason: SnackbarDismissReason) { | |
withAnimation { isVisible = false } | |
dismissHandlerBlock?(reason) | |
} |
buttonAction: {}, | ||
forceDismiss: true | ||
) | ||
assertSnapshot( |
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.
thanks for adding tests and avoiding generating several brands for this :)!!
@@ -18,7 +18,10 @@ struct SnackbarCatalogView: View { | |||
@State var buttonStyles: [SnackbarButtonStyle] = [.short, .large] | |||
@State var presentingSnackbar = false | |||
@State var presentingCrouton = false | |||
@State var autoDismissDelay = 3 | |||
@State var autoDismissDelay: CroutonDismissInterval = .fiveSeconds | |||
@State var intervalStyles: [CroutonDismissInterval] = [.fiveSeconds, .tenSeconds, .infinite] |
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's a constant so it shouldn't be @State
.
You can simplify this removing it and:
- autoDismissDelay: intervalStyles[selectedIntervalStyleIndex],
+ autoDismissDelay: CroutonDismissInterval.allCases[selectedIntervalStyleIndex],
wdyt?
β¦erval with/without an action. Added the same config to the crouton version in UIKit.
switch interval { | ||
case .fiveSeconds: | ||
return "5" | ||
case .tenSeconds: | ||
return "10" | ||
case .infinite: | ||
case .infinity: |
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.
infinite
sounds better than infinity
CHANGELOG.md
Outdated
@@ -8,7 +8,7 @@ | |||
|
|||
### BREAKING CHANGES | |||
|
|||
* **crouton:** DismissHandlerBlock has changed and now has a CroutonControllerDismissReason as return parameter | |||
* **crouton:** DismissHandlerBlock has changed and now has a SnackbarDismissReason as return parameter |
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.
don't change this, it's generated automatically ;)
Should we extend this https://github.com/Telefonica/mistica-ios/blob/main/Sources/Mistica/Components/Crouton/README.md ? Maybe something like: ConfigYou can use several intervals with different features:
|
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.
withText text: String, | ||
action: ActionConfig? = nil, | ||
config: SnackbarConfig, | ||
style: CroutonStyle = .info, |
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.
Nice! Managing one element with all the info, instead of multiple parameters
switch interval { | ||
case .fiveSeconds: | ||
return "5" | ||
case .tenSeconds: | ||
return "10" | ||
case .infinite: | ||
case .infinity(_): |
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 parameter is not needed, then it's enough with:
case .infinity:
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.
π SwiftUI Skills +1 π
π This PR is included in version 28.0.0 π The release is available on GitHub release Your semantic-release bot π¦π |
ποΈ Jira ticket
IOS-9604 Add infinite interval to Crouton/Snackbar SwiftUI versions.
π₯ What's the goal?
Add the same behaviour as in UIKit for crouton/snackbar SwiftUI versions with the infinite time invernal.
π§ How do we do it?
π§ͺ How can I verify this?
Launch the Snackbar SwiftUI version in the catalog and show it with different settings.
π AppCenter build